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

Add integration test(s) for Web #10563

Merged
merged 11 commits into from Aug 7, 2022

Conversation

LeeLenaleee
Copy link
Collaborator

Resolve:

@LeeLenaleee can you add another (the one you described here: #10552 (comment))?

Originally posted by @kurkle in #10553 (comment)

Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

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

I'd call this "react" or "react-browser" instead of "web" (in case we need to test different platforms)

test/integration/integration-test.cjs Outdated Show resolved Hide resolved
@kurkle
Copy link
Member

kurkle commented Aug 5, 2022

Maybe we also want to remove the "package-lock.json" so we always test with latest? (the one from node project needs to be removed for sure)
Also the .gitignore is a bit redundant because the tests are run in a temporary folder.

@LeeLenaleee
Copy link
Collaborator Author

Not sure if removing package-lock.json is a good idea, then we can run into issues where the test fails because the lib updated. If an unexpected bug comes in and break things that should not break the tests. I reckon the chance is low this will happen but it might be worth considering. After seeying what happend with our own package lately and how it broke the helpers import.

But removing it will indeed remove some maintance that will be caused because of needed updates.

Not sure what is the better tradeoff

@benmccann
Copy link
Contributor

I think the solution to the package-lock.yaml issue would be to use pnpm. That would allow you to use the workspace version of Chart.js while using locked versions of all other dependencies

@benmccann benmccann mentioned this pull request Aug 7, 2022
@etimberg etimberg merged commit 1551537 into chartjs:master Aug 7, 2022
@LeeLenaleee LeeLenaleee deleted the add-sample-web-integration branch August 8, 2022 11:47
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.

None yet

4 participants