Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[refactor] tests #1983

Closed
alexander-akait opened this issue Jun 6, 2019 · 8 comments
Closed

[refactor] tests #1983

alexander-akait opened this issue Jun 6, 2019 · 8 comments

Comments

@alexander-akait
Copy link
Member

alexander-akait commented Jun 6, 2019

No issue template. Ref: #1982 (comment)

Test structure (approximately):

- test
  - e2e
  - cli
  - client
    - clients
      - SockJSClient.test.js 
    - utils
      - sendMessage.test.js
    - overlay.test.js 
  - server
    - servers
      - SockJSServer.test.js
    - utils
      - tryParseInt.test.js

Test naming:

  1. class tests have same name as class -> MyClass.js -> MyClass.test.js
  2. pure file like defaultPort.js -> defaultPort.test.js
  3. options for server should be placed in server directory and have name compress-option.test.js
  4. options for client should be placed in client directory and have name sockPath-options.test.js

Next we should improve test performance, many tests are very long and written in effective way.

Some of tests files should be merge in other, some of separated based on above structure.

I am WIP on this after merge this PR


Ideally each option should have:

  1. Unit test (own)
  2. Integration test (Server.test.js)
  3. E2E test (can be avoided for non client options)
  4. CLI test (can be avoided right now)

We should concentrate on 1 and 2 points right now.

Other changes what we should do:

  • avoid using --runInBand it is decrease time on tests
  • union option.test.js and validation.test.js (they do same)
  • rewrite some test in more effective way
  • better naming of tests
@hiroppy
Copy link
Member

hiroppy commented Jun 6, 2019

Yes, I definitely agree with this plan.
I added below to this issue.

    - utils
      - sendMessage.test.js

@alexander-akait
Copy link
Member Author

Structure is done. Unfreeze merge for any branches.

/cc @hiroppy need rebase next

@hiroppy
Copy link
Member

hiroppy commented Jun 7, 2019

We don't rebase because a base branch will be broken if we force push. So I've always used cherry-pick. What do you think? (also, It depends on the committer to merge, but if the committer is not myself, then the commit revision is changed.) If you want to rebase next branch, I can fix.

@alexander-akait
Copy link
Member Author

Yes, we need rebase 👍

@hiroppy
Copy link
Member

hiroppy commented Jun 7, 2019

ok, so maybe pr will be broken(because of rebasing base branch)

@hiroppy
Copy link
Member

hiroppy commented Jun 7, 2019

And I deleted branch rules because if we set branch rules, we can not force push.

@alexander-akait
Copy link
Member Author

And I deleted branch rules because if we set branch rules, we can not force push.

👍

@hiroppy
Copy link
Member

hiroppy commented Jul 29, 2019

We completed migrating to the new directory structure. I'll close.

@hiroppy hiroppy closed this as completed Jul 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants