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

Swap the jest testing framwork for ava #18

Merged
merged 2 commits into from
May 13, 2020
Merged

Conversation

robertbrignull
Copy link
Contributor

The main purpose of this is a side project of mine to reduce the number of dependencies we have. It turns out that jest is absolutely humungous. This PR switches it for a more minimal testing framework called ava with no loss of functionality.

Don't get me wrong, jest is quite nice to use, but we don't use anything like its full feature set, so we can switch to a more minimal framework and still be able to do everything we want. I haven't used ava before but it seems reasonable and it's the most lightweight option I've found so far.

Some numbers:

  • The node_modules directory currently contains 16710 files. If we remove jest that come sdown to 2180 files. After adding ava in it goes back up to 5394, but still far below the original number.
  • The size of the zipped repository that is downloaded for every action run is currently 35MB. Swapping jest for ava reduces that to 15MB.
  • The code in the repository reduces by 1,339,989 lines.

The main change that was necessary to support this is that we need to now compile our test files to javascript before running them, while previousy we could run the typescript "directly" and it would be compiled for us. I think this is fine and it comes with the bonus that our linting rules now apply to our test files, where previously it appears there weren't.

Bonus point, the tests now pass for me locally, while they didn't before and only worked on actions. We used to fail because stuff like @actions/io doesn't work when outside of actions. I haven't 100% worked out why this is now working but I have a hunch it's because we are now compiling our test code to javascript.

To review, I've tried to keep all node_moduldes changes to the first commit, so you can skim through that. All the interesting changes are in the second commit.

Merge / deployment checklist

  • Run test builds as necessary. Can be on this repository or elsewhere as needed in order to test the change - please include links to tests in other repos!
    • CodeQL using init/analyze actions
    • 3rd party tool using upload action
  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.

@Daverlo
Copy link
Contributor

Daverlo commented May 7, 2020

If I checkout this branch and try to run np run-script test it fails:

(node:73266) ExperimentalWarning: Conditional exports is an experimental feature. This feature could change at any time

⠹ (node:73269) ExperimentalWarning: Conditional exports is an experimental feature. This feature could change at any time
⠸ ::debug::Ignoring location URI "/src/foo/bar.js' as it is outside of the src root
⠼ ::debug::Ignoring location URI "/src/foo/bar.js' as it is outside of the src root
::debug::Ignoring location URI "https:///Users/david/github/v2/codeql-action/lib/fingerprints.test.js' as the scheme is not recognised
::debug::Ignoring location URI "ftp:///Users/david/github/v2/codeql-action/lib/fingerprints.test.js' as the scheme is not recognised
::debug::Ignoring location as uri "1" is invalid
::debug::Ignoring location as uri "undefined" is invalid
::debug::Unable to compute fingerprint for non-existent file: /Users/david/github/v2/codeql-action/lib/fingerprints.test.js2
::debug::Ignoring location as index "1" is invalid
::debug::Ignoring location as index "0" is invalid
⠴ (node:73267) ExperimentalWarning: Conditional exports is an experimental feature. This feature could change at any time
⠋ fingerprints.ts › missingRegions

⠸ analysis-paths.ts › nonEmptyPaths
⠼ analysis-paths.ts › nonEmptyPaths
⠦ external-queries.ts › checkoutExternalQueries

  1 test failed

  external-queries.ts › checkoutExternalQueries


  Value is not `true`:

  false


@robertbrignull
Copy link
Contributor Author

Hmm, they work for me locally, and on actions. What's your nodejs version? I do remember reading something that this requires quite a new version. I'm on v12.16.3.

@Daverlo
Copy link
Contributor

Daverlo commented May 8, 2020

I'm on v13.8.0

@robertbrignull
Copy link
Contributor Author

Hmm, I've just installed v13.14.0 and it works for me.

Can you try again and post the error again? If it's a failure in checkoutExternalQueries then it could maybe be something to do you the tmp directory on your machine.

Copy link
Contributor

@Daverlo Daverlo left a comment

Choose a reason for hiding this comment

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

Now it works for me, I don't know what was the issue but it seems to be resolved

@robertbrignull
Copy link
Contributor Author

Somehow when rebasing to fix conflicts the churn has gone from +102,885 −1,442,871 down to +86,502 −1,442,275. This is all just random noise in the node_modules directory. All my changes to the package.json file are the same.

@robertbrignull robertbrignull merged commit d7b9f5a into master May 13, 2020
@robertbrignull robertbrignull deleted the remove_jest branch May 13, 2020 13:43
sampart added a commit that referenced this pull request Jun 15, 2020
Usage of Jest was removed in #18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants