Skip to content

Add integration test(s) for Web#10563

Merged
etimberg merged 11 commits into
chartjs:masterfrom
LeeLenaleee:add-sample-web-integration
Aug 7, 2022
Merged

Add integration test(s) for Web#10563
etimberg merged 11 commits into
chartjs:masterfrom
LeeLenaleee:add-sample-web-integration

Conversation

@LeeLenaleee

Copy link
Copy Markdown
Collaborator

Resolve:

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

Originally posted by @kurkle in #10553 (comment)

@kurkle kurkle left a comment

Copy link
Copy Markdown
Member

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)

Comment thread test/integration/integration-test.cjs Outdated
@kurkle

kurkle commented Aug 5, 2022

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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.

4 participants