chartjs / Chart.js

Simple HTML5 Charts using the <canvas> tag
https://www.chartjs.org/
MIT License
63.9k stars 11.88k forks source link

More detailed doc on how to contribute #11819

Closed koroliov closed 5 days ago

koroliov commented 6 days ago

Documentation Is:

Please Explain in Detail...

is there any more detailed documentation on how to contribute? I see this one: https://www.chartjs.org/docs/latest/developers/contributing.html but it's very general. E.g. I can't figure out what should I do to make the tests pass.

  1. I ran pnpm test
  2. I had lots of warnings
  3. Firefox didn't pass (a single test fails and karma bails out)

I expected first to make everything run correctly, then discuss the changes I'd like to contribute

But now I'm being stuck on the tests. And I'm not sure if I have to make all the tests pass, or is it normal (may be other people also have failing tests, warnings). The general doc doesn't answer this.


Put another way: how to deal with failing tests, warnings on pnpm run ? What will happen if tests pass in my environment but don't in someone else's one?

Your Proposal for Changes

Someone in a position of authority explains the asked questions.

Example

No response

LeeLenaleee commented 5 days ago

The tests should pass, at least in the CI otheriwse a pr can't be merged. Locally all the test work for me. Which test fails for you?

For the warnings, you should try to write new code so that there are no new warnings. Some files already have them and then you can just work around those.

koroliov commented 5 days ago

For the warnings, you should try to write new code so that there are no new warnings

It's rather hard when you have this many warnings to tell if there are new ones. Although as I see most (if not all) of them are related to some 'Filler plugin' being not registered.

Screenshot from 2024-06-27 15-38-35

Which test fails for you?

Looks like this one, always the same (may be there are others, which won't pass, but currently this is the only one):

[karma] Chart.elements.PointElement [karma] auto [karma] ✗ /base/test/fixtures/element.point/rotation.js [karma] Fixture test failed: [karma] Difference: 1732px / 1.32% [karma] Threshold: 10% [karma] Tolerance: 0.1% [karma] [karma] specFromFixture/</</_done/<@node_modules/.pnpm/chartjs-test-utils@0.4.0_jasmine@3.99.0_karma-jasmine@4.0.2_karma@6.4.1/node_modules/chartjs-test-utils/dist/chartjs-test-utils.js:1086:25 <- test/index.js:1091:27

I'll try to figure out what's the reason.


Could you answer please which operating system do you use, and what's your screen resolution ?

I use Fedora Linux on a rather small laptop. Like 13', 1920 x 1080

koroliov commented 5 days ago

Now I ran tests for both Chrome and FF. A different FF test failed. (Previously I ran only for FF by editing the karma config file).

[karma] Core.scale [karma] auto [karma] ✗ /base/test/fixtures/core.scale/tick-backdrop-rotation.js [karma] Fixture test failed: [karma] Difference: 3398px / 5.18% [karma] Threshold: 10% [karma] Tolerance: 0.16% [karma]

koroliov commented 5 days ago

First, I'll try to use a wider monitor. I have a 24' one

koroliov commented 5 days ago

@LeeLenaleee the tests which fail seem to be random. Also the 24' display hasn't helped, tests still fail in FF. Could you tell me which OS are you using

koroliov commented 5 days ago

@LeeLenaleee and which node version are you using? I have 22.2.0 And pnpm --version is 9.4.0

koroliov commented 5 days ago

@LeeLenaleee Hi, I tweaked slightly the test configs and:

LeeLenaleee commented 5 days ago

I don't know what you want to do but I would just leave the test's alone for now and wait for the CI to see what happens if you plan to make a small change

koroliov commented 5 days ago

Thank you, I guess, it's the most convenient and reasonable way to proceed. At least Chrome passes, so I can test just in it for a time being. @LeeLenaleee let me ask one more question (not even related to this opened issue): where can I discuss with someone in a position of authority the planned changes. I'd like (as the doc advises) to have opinions/feedbacks beforehand: if it's worth it, should it be a plugin, how the api should look etc.

LeeLenaleee commented 5 days ago

The best thing you can do is to just open a issue with your proposed changes I think

koroliov commented 5 days ago

I'm closing this, b/c for me personally the issue has been resolved