Skip to content

Migrate no-workspace integration tests to Jest#1782

Merged
koesie10 merged 10 commits intojest-migration/integration-testsfrom
jest-migration/no-workspace
Nov 24, 2022
Merged

Migrate no-workspace integration tests to Jest#1782
koesie10 merged 10 commits intojest-migration/integration-testsfrom
jest-migration/no-workspace

Conversation

@koesie10
Copy link
Copy Markdown
Member

@koesie10 koesie10 commented Nov 22, 2022

This migrates all no-workspace VSCode integration tests to Jest by running npx jest-codemods, followed by manual fixes (mostly for Sinon and Proxyquire).

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@koesie10 koesie10 changed the base branch from main to jest-migration/integration-tests November 22, 2022 14:56
@koesie10
Migrate no-workspace integration tests to Jest
a0ab34b 
This migrates all no-workspace VSCode integration tests to Jest by
running `npx jest-codemods`, followed by manual fixes (mostly for Sinon
and Proxyquire).
@koesie10
Do not run integration tests in parallel
3da0034 
This results in parallel downloads of VSCode, which is not supported.
@koesie10
Set timeouts on tests correctly
65bb61b 
The timeouts need to be set either on a per-file basis, or per test by
using the parameter in `it`. Since we have both Mocha and Jest types, we
need to declare in the test file which one we're using.
@koesie10
Move jest-runner-vscode config to correct location
0296db8
@koesie10
Use correct jest-runner-vscode for no-workspace
90dd97f
@koesie10
Use different method of running tests
a985bcb
@koesie10 koesie10 force-pushed the jest-migration/no-workspace branch from e370f26 to a985bcb Compare November 23, 2022 10:23
@koesie10 koesie10 marked this pull request as ready for review November 23, 2022 13:21
@koesie10 koesie10 requested review from a team as code owners November 23, 2022 13:21
robertbrignull
robertbrignull reviewed Nov 23, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@robertbrignull robertbrignull left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me, although with the caveat that I'm not an expert on Jest and I haven't been following along with the conversion chat as much as other people have been. I also didn't read the auto-generated commit, but I read the rest of the commits, and the resulting code looks pretty good.

The windows tests seem to be failing. Is that an unrelated error or something to do with this PR?

So long as the tests pass and the auto-conversion looks somewhat sensible, is it better to get a quick review and merge this PR to minimise disruption?

@koesie10
Copy link
Copy Markdown
Member Author

koesie10 commented Nov 23, 2022

Thanks for the review!

The windows tests seem to be failing. Is that an unrelated error or something to do with this PR?

That is related to this PR. The jest-runner-vscode doesn't seem to properly support running a "non-standard" configuration on Windows, so we aren't currently able to run these tests on Windows. See the internal linked issue for more details.

So long as the tests pass and the auto-conversion looks somewhat sensible, is it better to get a quick review and merge this PR to minimise disruption?

I think that is indeed the best course of action. The PR is going to a feature branch anyway and I think the configuration can be re-used by other integration test directories, so it's valuable to have those in there.

charisk
charisk approved these changes Nov 23, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@charisk charisk left a comment

Choose a reason for hiding this comment

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

Generally looks good to me. I can't say I've looked at all the changes though... there's A LOT!

When running locally I get 1 error - launching with no specified workspace › should not activate the extension at first, but happy for us to look at that separately.

Comment thread extensions/ql-vscode/package.json Outdated
Comment thread extensions/ql-vscode/src/vscode-tests/jest.setup.ts Outdated
Comment thread extensions/ql-vscode/src/vscode-tests/no-workspace/activation/activation.test.ts Outdated
Comment thread extensions/ql-vscode/src/vscode-tests/no-workspace/contextual/queryResolver.test.ts Outdated
Comment thread extensions/ql-vscode/src/vscode-tests/no-workspace/query-history.test.ts Outdated
@koesie10
Use resetMocks option to reset mocks before every test
043a7b1
@koesie10
Use projects arg to run the Jest tests
347d234
@koesie10
Clarify comment for activation test
01f6576
@koesie10
Remove calls to fail
e26e111 
Instead of calling `fail` and catching an error, this will use the
`rejects.toThrow` assertion to check that a Promise rejects with a
specific error.
@koesie10
Copy link
Copy Markdown
Member Author

koesie10 commented Nov 24, 2022

Thanks for the review!

When running locally I get 1 error - launching with no specified workspace › should not activate the extension at first, but happy for us to look at that separately.

Have you removed the out directory prior to building/watching this branch? The activation.test.ts file has been moved to a separate directory and building/watching doesn't actually clear the out directory, so it could be that there is still a file out/vscode-tests/no-workspace/activation.test.js, which should now be located at out/vscode-tests/no-workspace/activation/activation.test.js.

@koesie10 koesie10 merged commit c31635f into jest-migration/integration-tests Nov 24, 2022
@koesie10 koesie10 deleted the jest-migration/no-workspace branch November 24, 2022 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@robertbrignull robertbrignull robertbrignull left review comments

@charisk charisk charisk approved these changes

Assignees

No one assigned

Labels

secexp

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@koesie10 @charisk @robertbrignull