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

deps/upgrade webauthn test support #2747

Merged
merged 7 commits into from
Apr 22, 2024

Conversation

deen13
Copy link
Contributor

@deen13 deen13 commented Apr 16, 2024

Short Description

This pull request configures vitest and adds a simple test as suggested in my review of deps/upgrade_webauthn_2_0.

Proposed Changes

  • Added a test task to package.json
  • Included required vitest dependency
  • Implemented a simple test for URL and base64 encoding
  • Configure vitest to ignore python dependencies
  • Configure import/resolver to resolve imports correctly
  • Add job to circleci workflow

Pull Request Review Guidelines

@deen13 deen13 requested a review from a team as a code owner April 16, 2024 10:07
@deen13 deen13 changed the title Deps/upgrade webauthn test support deps/upgrade webauthn test support Apr 16, 2024
@deen13 deen13 force-pushed the deps/upgrade_webauthn_test_support branch from 092bf6e to e12d995 Compare April 18, 2024 21:03
Copy link
Member

@david-venhoff david-venhoff left a comment

Choose a reason for hiding this comment

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

Thanks 🎉
I think it is a great step to enable js testing. It seems like this pr would close #2583, so it probably makes sense that @seluianova also takes a look at this.

When I tested this pr, I got an error that vitest exceeds the maximum number of file watchers on my system, because it seems to be watching every file in this repo (Including files of dependencies) for some reason. I managed to get rid of the error using this vitest.config.ts file:

import { defineConfig } from 'vitest/config';

export default defineConfig({
  test: {
    include: ['integreat_cms/static/src/js/__tests__/**'],
    watch: false,
  }
})

But I find it very weird that the watch: false option seems to be required to prevent this error, and that the glob is not enough to prevent it from watching every file.

Apart from that I think it makes sense to run npm test as part of our ./tools/test.sh, or is there a reason against that?

david-venhoff and others added 3 commits April 20, 2024 18:00
Watching files in .venv is not required,
since we don't have any tests there and it can also
 lead to problems on platforms that only allow a
limited number of file watchers.

Co-authored-by: deen13 <[email protected]>
@david-venhoff david-venhoff force-pushed the deps/upgrade_webauthn_test_support branch from 7479405 to 8506b64 Compare April 20, 2024 16:00
@deen13
Copy link
Contributor Author

deen13 commented Apr 20, 2024

Just for the record: @david-venhoff and I successfully resolved the mentioned issue in commit 09be3e5.

@deen13
Copy link
Contributor Author

deen13 commented Apr 20, 2024

Apart from that I think it makes sense to run npm test as part of our ./tools/test.sh, or is there a reason against that?

That's a good point @david-venhoff! I took a look at ./tools/test.sh and noticed that it has quite a few options that it passes on to pytest like the option to invoke a single test (./tools/test.sh tests/cms/my_test.py -k my_test_case). I would prefer to keep this file as is because it is tailored to pytest, and more importantly, I don't want to break anyone's workflow. Do you think it's worth adding another script that runs all python and ts tests but accepts no flags?

@deen13
Copy link
Contributor Author

deen13 commented Apr 20, 2024

It seems like this pr would close #2583, so it probably makes sense that @seluianova also takes a look at this.

I believe the pull request only partially resolves the issue because it focuses solely on adding unit tests, whereas the mentioned issue pertains to UI tests. Nevertheless, I'm eager to contribute to UI testing if it's still necessary. Otherwise, feel free to disregard my comment.

@seluianova
Copy link
Contributor

I believe the pull request only partially resolves the issue because it focuses solely on adding unit tests, whereas the mentioned issue pertains to UI tests

@david-venhoff right, in UI tests we wanted to add end-2-end kind of tests, that simulate user interaction. Like opening the browser, filling in fields, clicking on buttons, etc. At least that was my understanding of the task

@david-venhoff
Copy link
Member

david-venhoff commented Apr 21, 2024

Ah okay 👍

I would prefer to keep this file as is because it is tailored to pytest, and more importantly, I don't want to break anyone's workflow. Do you think it's worth adding another script that runs all python and ts tests but accepts no flags?

Imo in an ideal world, we would have the parameters that can be passed to test.sh automatically work for both python and ts tests. But that seems complicated enough to be separated into its own pr, so I think what you propose is fine 👍

@deen13 deen13 merged commit cb82c06 into deps/upgrade_webauthn_2_0 Apr 22, 2024
1 check passed
@deen13 deen13 deleted the deps/upgrade_webauthn_test_support branch April 22, 2024 06:59
@deen13 deen13 restored the deps/upgrade_webauthn_test_support branch May 21, 2024 14:34
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