[L&D] Randomise jasmine tests #6267
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mpw5
force-pushed
the
ctskf-445-randomise-jasmine-tests
branch
from
2b7f8a6 to
c6e8c99
Compare
mpw5
force-pushed
the
ctskf-445-randomise-jasmine-tests
branch
3 times, most recently
from
cfbeb6d to
d8c2ae3
Compare
Katy600
reviewed
Dec 15, 2023
| @@ -112,7 +114,7 @@ describe('Modules.SideBar.js', function () { | |||
| }) | |||
|
|
|||
| it('should have a `phantomBlockList` property defined', function () { | |||
| expect(moj.Modules.SideBar.phantomBlockList).toEqual(['fixedFees', 'gradFees', 'miscFees', 'warrantFees', 'interimFees', 'transferFees', 'disbursements', 'expenses']) | |||
| expect(moj.Modules.SideBar.phantomBlockList).toEqual(['fixedFees', 'gradFees', 'miscFees', 'warrantFees', 'interimFees', 'transferFees', 'hardshipFees', 'disbursements', 'expenses']) | |||
There was a problem hiding this comment.
@mpw5 this is just me attempting to code review so please ignore if not important. Is it possible to change the setup so that you don't have to make this change twice? Or is that an unnecessary refactor and a faff?
There was a problem hiding this comment.
Thanks Katy. It's a good question. I guess it could be optimised slightly to remove the repetition and I think if that list of fees was being used in more places then I probably would pull it out into a variable so that we could re-use it, but as it's only being used once in the setup and once in the expectation it doesn't feel too bad.
Katy600
approved these changes
Dec 15, 2023
The jasmine tests for javascript code are not currently randomised. It is desirable for tests to run in a random order as each test should be isolated and independent of other tests. This commit enables the `random` environment variable in `spec/support/jasmine-browser.json`. This causes tests to start failing (highlighting why this is a problem). These failures will be fixed in subsequent commits.
When the jasmine test suite is run in a randomised order, failures
intermittently occur in tests in `spec/javascripts/Modules.AllocationDataTable_spec.js`.
There are two reasons for this:
1) In order for the tests to pass, `module.init()` needs to be called
before tests in the `...setAjaxURL` describe block are run, rather than
just relying on it having been called in previous tests. This commit adds
that call in a beforeEach block.
2) Errors also occur because a test in `Modules.AllocationScheme_spec.js`
changes the filtered scheme from AGFS to a dummy test value ('input-value').
When this test runs before the tests in `Modules.AllocationDataTable_spec.js`,
this change in state leaks through where tests expect the scheme to be AGFS,
leading to a failed test.
This commit fixes this in two ways:
* it adds teardown code to the test in `Modules.AllocationScheme_spec.js` to
reset the filter after the test has completed;
* it adds a call to `module.init()` in `Modules.AllocationDataTable_spec.js`
to ensure that the page is correctly set up before running the test.
This commit also DRYs up the code a little by using already-defined
`consts` instead of full module definitions.
When running jasmine tests in a randomised order, tests in `spec/javascripts/helpers-sidebar_spec.js` intermittently fail. This is because some tests in that file are not torn down correctly and so affect the running of other tests. This commit fixes the problem by ensuring that the `.js-block` element is reset after each test has run.
Fixes `afterEach` so that it removes the `disbursements-view` element created in `beforeEach`, rather than the non-existent `offences-view`.
When running jasmine tests in a randomised order, tests in
`spec/javascripts/modules-DuplicateExpenseCtrl_spec.js` intermittently
fail. This is because some tests in that file are not setup and torn down
correctly and so affect the running of other tests.
This commit fixes the problem by reinstating a `$('body').append(domFixture);`
step which appears to have been erroneously commented out.
It also removes the `domFixture.empty()` from the `beforeEach` block
and changes the `afterAll` function (which already calls that method
to `afterEach`, as it makes more sense to tear down the previous test
after it has run, rather than at the start of the next test.
Updates tests in modules-sidebar_spec.js to include hardshipFees, which appears not to have been fully updated when hardship fees were added to SideBar.js in 2020.
* Removes several calls to `jsBlockFixtureDOM.empty()` which are unnecessary
as they are done by the afterAll function on line xxx.
* In `describe('...calculations'...`, appending `jsBlockViewCalculated` to
`$(#claim-form)` does not have the desired effect. Instead it is necessary
to append `jsBlockViewCalculated` to `jsBlockFixtureDOM`, and then append
that to ` $('body')`.
* Adds some missing setup to `describe('...bindListeners'...`, using the
same pattern of appending used above.
* Adds a name attribute to the mock 'Save as draft' button defined in `fixtureDom` so that it can be clicked in the appropriate test * Moves the definition of `submitCallback` to the `beforeEach` function so that it is reset between tests * Adds an `afterEach` function to the 'should not alert when copy of the indictment is selected in the supporting evidence checklist' test, to ensure that the indictment checkbox is reset after the test has run and so does not affect anysubsequent tests
* Moves `$('body .mod-search-input').remove()` to an afterEach function,
instead of calling it as the first step of the beforeEach function. This
previous approach meant that the last test in this file was not torn
down correctly and caused subsequent tests to fail.
* Fixes the test defined in `describe('...trackUserInput'...` which
fails because `module.$input.val('mudr')` is called before module has
been initialised. This is done by calling `module.init()` before
`module.$input.val('mudr')`.
mpw5
force-pushed
the
ctskf-445-randomise-jasmine-tests
branch
from
d8c2ae3 to
c5e7cd0
Compare
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
CCCD contains a suite of jasmine tests which unit test the javascript code. These run in the same order each time, which hides some issues with the tests. This PR enables randomised running of the tests and fixes those issues.
Ticket
CTSKF-445
Why
It is desirable for tests to run in a random order as each test should be isolated and independent of other tests. Running the tests in a random order causes tests to start failing, highlighting why this is a problem.
How
randomattribute inspec/support/jasmine-browser.jsontotrue.There are still a number of improvements that could be made to the jasmine tests (eg ensuring that all code is covered by tests; improving consistency between tests; removing jQuery; rewriting the tests in a more modern style as the javascript is modernised - see #6191 for example) but are out of scope for this PR.