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

Custom jest environment - Allow ES Class unit tests #3428

Merged
merged 2 commits into from
Jul 11, 2023

Conversation

oliverfoster
Copy link
Member

@oliverfoster oliverfoster commented Jun 6, 2023

Fix

  • Reduced react and react-dom versions to 17.0.2 to be inline with the adapt varieties
  • Removed mocks for adapt es classes and used classes instead

New

  • Created and used jest-environment-adapt to load scorm_test_harness.html and all associated files, up until adapt.min.js, whereby tests can import Adapt from 'core/js/adapt' etc, and test the code directly.
  • Mocked backbone, html-react-parser to steal from the Adapt loaded versions
  • Mocked bowser to override appropriately
  • Mocked plugins dynamically, to allow import 'core/js/app'; to load the compiled plugins, which will allow loading a full version of adapt into the testing environment for more complex json configured tests

Testing

  • One must run grunt dev before running npm run jesttest as files are loaded from the build/ folder to setup the test environment
  • Libraries are loaded in a use strict environment (have yet to figure out how to stop this), so mediaelement-and-player.js has some delete statements which fail
  • adapt-contrib-spoor.js uses require from the global scope, it should use window.require instead
  • wrapper.js errors when trying to find Adapt.course as it's not loaded yet, so a return early must be inserted to fix

image

@oliverfoster oliverfoster self-assigned this Jun 6, 2023
@cahirodoherty-learningpool
Copy link
Contributor

cahirodoherty-learningpool commented Jun 7, 2023

Great work Oliver!
I was wondering what the maintenance plan for jest-environment-adapt would be? Is it at all possible to bundle it directly in with the FW code?

Also, to handle the build/ folder requirement, could the package.json script be updated to
"jesttest": "grunt dev && jest",

Copy link
Contributor

@eleanor-heath eleanor-heath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is fantastic, so happy to see :)

@oliverfoster
Copy link
Member Author

oliverfoster commented Jun 7, 2023

Great work Oliver! I was wondering what the maintenance plan for jest-environment-adapt would be? Is it at all possible to bundle it directly in with the FW code?

I don't think it's possible to bundle the code directly, as one has to specify a module name in the config, I'll have a look see if there are alternative ways of extending the jest-environment-jsdom module which are more integrated. This code will be fw version specific, but its version is specified in the fw package.json, so it shouldn't be too difficult to maintain alongside. I will transfer into the adaptlearning org if we're in agreement that this approach looks good enough.

It's not finished at the moment, I'd like all of the hardcoded strings to be configurable from the jest config. The build/ folder should be configurable in a similar way to the --outputdir= for grunt.

Also, to handle the build/ folder requirement, could the package.json script be updated to "jesttest": "grunt dev && jest",

Probably grunt diff as it doesn't wait at the end. The jesttest command is under contention from my comments on the main pr. I think we should have one unified testing api, even though we're using two frameworks to pull it off. npm test unit and npm test e2e rather than npm run jesttest and npm test. This pr doesn't contain that work.

document.body.appendChild(container);
Adapt.course = {
get: jest.fn().mockReturnValue(
Copy link
Member Author

@oliverfoster oliverfoster Jun 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eleanor-heath I'm pretty sure there is a much better way of mocking this Adapt.course.get function, but I didn't manage to read that far into the jest api. The Adapt.course object doesn't exist here, because Adapt data hasn't loaded, because core/js/app wasn't imported. You can see the commented out section above which does that. (You can see my poor attempt at an async "wait for condition", to wait for _isStarted, thinking about it, Adapt.on('change:_isStarted', () => resolve(Promise.resolve())) would have worked).

@oliverfoster
Copy link
Member Author

We should probably have a meeting to discuss what this code actually does?

@eleanor-heath
Copy link
Contributor

We should probably have a meeting to discuss what this code actually does?

Yes please, I read your previous comment here #3383 (comment) so understand a lot of what has been done based on the comment you have added but a call to discuss unit testing in general and our plans would be ideal.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀

Copy link
Contributor

@joe-allen-89 joe-allen-89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really good work all involved on this, it is fairly straight forward to get up to speed with and works as expected, going to be very useful going forward.

@oliverfoster oliverfoster merged commit a5804fc into Framework-unit-testing Jul 11, 2023
@oliverfoster oliverfoster deleted the issue/3417 branch July 11, 2023 08:45
cahirodoherty-learningpool pushed a commit that referenced this pull request Sep 26, 2023
…3383)

* Issue#64/Introduce Jest component testing to the framework starting with Core Header template

* issue#64/reverted change to adapt-contri-core

* Issue#64 - removed view and model mock

* Issue#64 - modified mocks to only include what was needed for the test, added spy on isMinScreenSize to test different screen sizes, added jest to eslintrc and added extra clearjest cache command

* Issue#64 - removed device.screenSize setting within test as device.screenSize is no longer in use within the header template

* Issue#64 - header file path update after initialising the core submodule

* Removed coverage reports

* Git ignore all files within the coverage directory

* Added package-lock

* removed displayTitle test

* isScreenSizeMin Mock const

* removed react helpers extported templates as is it not used in the jsx template

* Extended with a custom jest environment to allow ES Class unit tests (#3428)

* Fix: files moved to test folder, test preperation/execution seperated.

* Files removed from root after being added to test folder.

* Refactor

* Pull jest-environment-adapt from npm instead of github

* Added npm test help

* Updating testing environment to 16 and removed typo

---------

Co-authored-by: Eleanor Heath <[email protected]>
Co-authored-by: Oliver Foster <[email protected]>
Co-authored-by: joe-allen-89 <[email protected]>
github-actions bot pushed a commit that referenced this pull request Sep 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants