Tests for Util Module

[reficul31]

Refs
Some of the tests in this module are coded to be skipped due to some technical issues at the jest repository. Refer. Although it will be fixed once we upgrade to Jest22.

I would like to clear some doubts regarding the general construction of the testing suite.

• Is the following style of test structure appropriate?

describe('[moduleName]', () => {
    test('should [describe behaviour]', () => {
    })
})

• Again, to rediscuss the issue of having tests in their own directory of __tests__ in comparison to having them beside the module?

• In this PR I have written a combination of Positive and Negative tests. Would the devs like to follow the Positive style of testing or the Positive-Negative style of testing?

I have chosen to test the util module to be fully tested first as it is completely decoupled from the whole project. If we test the whole util module it would allow us to just pick up and use any function from this module without worrying too much about the implementation.

[poltak]

This is awesome @reficul31! Thanks a lot for getting started on this.

To reply to some of your points:

it will be fixed once we upgrade to Jest22

Feel free to upgrade test-related deps on this branch - shouldn't effect anything existing.

Is the following style of test structure appropriate?

Seems fine! Reading through some of your tests, it's all very understandable. Will look over more thoroughly later

rediscuss the issue of having tests in their own directory of __tests__

Same opinion as here, although tbh I'm not really fussed either way. What benefits would there be from having a separate dir tree for tests? Any personal recommendation?

Would the devs like to follow the Positive style of testing or the Positive-Negative style of testing?

I think positive cases are more important, but negative test cases would be very welcome in any module that expects some sort of user-defined input. Any suggestion?

Some other things:

1. Think we gotta set up eslint to work with jest, or ignore .test files in non-test npm scripts, or something (maybe eslint-plugin-jest is not setup properly) - should be simple
2. you said the other night about maybe using mocha + whatever other individual testing libs rather than jest as a whole. You wanna stick with jest? (I have no strong opinion, just want to know so I can get more familiar with jest - choice is yours)

[reficul31]

Thanks for replying quickly @poltak . To follow up some questions and answer some queries.

What benefits would there be from having a separate dir tree for tests? Any personal recommendation?

I would actually prefer the unit tests to be placed beside the modules and the integration tests placed in their own folders. This would allow us to run the testing suite simultaneously for the unit tests and the integration tests as integration tests would take more time.(We could also separate the snapshot tests of react. But that's a bridge we can cross once we get there.)

Think we gotta set up eslint to work with jest, or ignore .test files in non-test npm scripts, or something (maybe eslint-plugin-jest is not setup properly) - should be simple

Oh. I thought the plugin was setup. I will set it up and maybe send it in another PR?

I think positive cases are more important, but negative test cases would be very welcome in any module that expects some sort of user-defined input. Any suggestion?

For sure, positive test cases are the most important and forms the basis of test based programming. I think we can add negative test cases in some modules to check and see what sort of behaviour is take up by it functions. Eg. Here are some examples of the negative test cases in a module.

you said the other night about maybe using mocha + whatever other individual testing libs rather than jest as a whole. You wanna stick with jest? (I have no strong opinion, just want to know so I can get more familiar with jest - choice is yours)

Yeah I thought a lot about using mocha and its many many tools. But due to a variety of reasons I chose to go with jest.

• First, jest was already setup and just waiting for the tests to be plugged in.
• Already a lot of work went into making the test suite in jest. It would be cumbersome to translate all that work again into another framework.
• Great docs for jest! Although the community housed by Mocha makes up for it and then some.
• Mocha is a bare bone framework and would require us to choose the tools individually, this would require additional discussion and therefore I thought choosing jest would save code review, discussion as well as programming time.

Should I send the unit tests in batches and group them according to the modules or would it be much more easier to send individual files? I personally would prefer sending individual files so as to not clutter the discussion with too much code review. But I am open to suggestion.

[poltak]

I will set [eslint with jest] up and maybe send it in another PR?

I think it's probably just a line in the conf or some dep - best to add in her,e as we don't really want to merge anything in that introduces lint issues (should be added as a failable CI stage when we get to that later).

No issue with any of your points.

Should I send the unit tests in batches and group them according to the modules or would it be much more easier to send individual files?

Does this mean grouping multiple module's unit tests in a single .test.js file vs one file per module? If so, I'm for file one per module. If that's not what you meant, let me know

[reficul31]

Does this mean grouping multiple module's unit tests in a single .test.js file vs one file per module? If so, I'm for file one per module. If that's not what you meant, let me know

Sorry for the confusion. No, I just meant should I send the PRs containing tests for the whole modules like this one? Eg(Tests for util or tests for activity logger). Or should I separate the PRs by sending 1 PR for each file in a module? Eg(Tests for util/delay in one PR and similarly for other files)

I would prefer having one PR for each file in the module. This way the code review won't get cluttered with all the inputs from all the files in the module. Also I feel it would be much more easier to review just one test at a time, as reviewing one full module of tests is a bit tedious. But I am open to suggestions.

[reficul31]

Short Update:

• Rebased the PR
• Added the linter. Refer for a thorough examination of the different ways the linter could be integrated with the jest presets.

[poltak]

I would prefer having one PR for each file in the module

sure, send your PRs as you like :)

The skipped tests - for tab-events and webextensionRPC modules - throw linting warns because of rule jest/no-disabled-tests. Are those skipped tests because of the problem with jest version you said in OP? Feel free to disable that rule if there is a reason to keep the skipped tests, or just ignore if you're planning on updating the tests

[reficul31]

Are those skipped tests because of the problem with jest version you said in OP? Feel free to disable that rule if there is a reason to keep the skipped tests, or just ignore if you're planning on updating the tests

Yes the test are skipped for the same reason as mentioned above. I would have disabled the warning but because it is short term, it is better to keep it. The new version of jest release will mostly take care of the problems in the tests. Actually, it came to my notice that the new version was made available 3 days ago. Just waiting for them to smooth out some of the bugs and then I will update.
Jest Releases

[poltak]

How does await null work? Wouldn't it just continue on immediately? Or is it similar to passing 0 to setTimeout, putting the subsequent code at the end of the execution queue?

[reficul31]

Here is some context, I hope this provides some help regarding the clarification of the use of await null. I am still searching for a better alternative. Hopefully the upgrade in jest might just resolve the issue.

[reficul31]

@poltak Almost done?

[poltak]

What's the status of this now? I'm happy to merge it in whenever you're finished making the changes you wanted to do

…

On Jan 10, 2018, at 16:36, Shivang Bharadwaj ***@***.***> wrote: @poltak Almost done? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[reficul31]

I was thinking we could merge these for now and then I could send individual pull requests for the remaining files in the utils module so that it is easier to review and discuss?
I am happy with the state of tests in this PR and would be happy to merge these in.

[poltak]

Great stuff @reficul31 . We should add our test npm script into our precommit hook as well now.

[reficul31]

I am not completely sure about the pre commit hook for the test script. Since I was hoping to setup a CI as soon as the unit testing suite is up and running. Would running the tests twice in a pre commit hook as well as in a CI be counter intuitive?
Also some of the test that will be written at a later stage will be quite heavy and would probably take some time to run.(Eg. Integration tests) I am not completely sure though.

[poltak]

Would running the tests twice in a pre commit hook as well as in a CI be counter intuitive?

Yeah if you're gonna set up a CI server to handle all that, I think it should be fine to leave out of precommit.