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

Fix chartjs warnings #5964

Merged
merged 3 commits into from
Feb 20, 2024
Merged

Conversation

tsmock
Copy link
Contributor

@tsmock tsmock commented Jul 6, 2023

See #5721 (comment) for additional context.

Big things:

  • Keep current behavior as much as possible
  • ISO 8601 YYYY-MM-DD for chart X axis labels
  • Tooltips shouldn't show hour/minute/second
  • I ended up expanding the tooltips a bit ("Thursday, July 6, 2023" instead of "2023-07-06").

@tsmock tsmock force-pushed the fixup/chartjs-warnings branch 2 times, most recently from 1ef07da to c58569c Compare July 7, 2023 09:43
HelNershingThapa

This comment was marked as resolved.

@tsmock
Copy link
Contributor Author

tsmock commented Jul 10, 2023

That is funny. It wasn't giving me that warning when I was working on it. And it is also erroring out for me too. Which makes me think I forgot to run yarn install or something.

Anyway, I'll see if I can fix this today.

@github-actions github-actions bot added dependencies Pull requests that update a dependency file javascript labels Jul 10, 2023
@tsmock
Copy link
Contributor Author

tsmock commented Jul 10, 2023

It should be fixed now. When I was working on this, chartjs was sending the tick value instead of the tick label to the calllback, and something changed somewhere so that the tick label was sent instead of the tick value. So instead of something like "Jun 1" being sent to the callback, I was getting 1685599200000. So I must have been using an outdated/borked version of a dependency somewhere.

@HelNershingThapa
Copy link
Contributor

Getting the following error now:

./node_modules/chart.js/dist/chart.js 559:18
Module parse failed: Unexpected token (559:18)
File was processed with these loaders:
 * ./node_modules/react-scripts/node_modules/babel-loader/lib/index.js
You may need an additional loader to handle the result of these loaders.
| };
| class DatasetController {
>   static defaults = {};
|   static datasetElementType = null;
|   static dataElementType = null;

@tsmock
Copy link
Contributor Author

tsmock commented Jul 10, 2023

It looks like I had some stuff from #5908. Its probably the chartjs update that did that. I'll back that out and check and see if it works properly.

@github-actions github-actions bot removed javascript dependencies Pull requests that update a dependency file labels Jul 10, 2023
@tsmock
Copy link
Contributor Author

tsmock commented Jul 10, 2023

It should be fine now (tested with rm -rf node_modules && yarn install --check-files && yarn build && yarn start).

@sonarcloud
Copy link

sonarcloud bot commented Jul 10, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.1% 0.1% Duplication

Copy link
Contributor

@HelNershingThapa HelNershingThapa left a comment

Choose a reason for hiding this comment

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

Thank you for fixing those warnings. I like how the tooltip format has been modified; it provides a more user-friendly touch. Although I personally preferred the display of months instead of YYYY-MM-DD, I appreciate the changes nonetheless.

@tsmock
Copy link
Contributor Author

tsmock commented Jul 11, 2023

preferred the display of months instead of YYYY-MM-DD

I can see it going either way. With that said, the test project I was using had 3 tasks mapped on different days in one month, and then a task mapped in that same month the next year. So the labels were June 2023 ---- June 2023 ---- June 2023 ---- June 2024. Which made no sense to me, so I changed it back to the YYYY-MM-DD format (which is where the ticks.callback function comes from).

@HelNershingThapa
Copy link
Contributor

CircleCI is consistently failing (9 times) because of a failing test case with src/views/tests/project.test.js (13.625 s) ● Project Detail Page › should render component details, so I've been holding this off from being merged.

@tsmock
Copy link
Contributor Author

tsmock commented Jul 11, 2023

FML. It worked both in the IDE and on the command line for me. I'll try multiple runs and see if it appears so I can debug it. :(

EDIT: It took 16 runs on the command line until it reproduced, "FAIL src/views/tests/taskSelection.test.js (34.597 s) \r\n ● Task Selection Page › should change the button text to map selected task when user selects a task"

I bet this is a timing issue or something stupid like that. :(

@HelNershingThapa
Copy link
Contributor

I completely understand your frustration. This is the same test case issue we've been battling for a while now. In the past, CircleCI builds would typically pass, but unfortunately, this time, it didn't.

Debugging this issue has been a headache for me as well. I think I'll take a shot at fixing warnings in src/views/tests/project.test.js in the hope that they magically start passing.

@tsmock
Copy link
Contributor Author

tsmock commented Jul 12, 2023

If you are looking at fixing the forwardRef warnings, see 0de7dd2 and (maybe, I don't remember if this actually fixed it) cd758ca for the memory leak issue.

@HelNershingThapa
Copy link
Contributor

I had no intention of repairing those. I was aware that you had handled it on the larger PR, so I was looking for other warnings in 930545c . My attempt was futile; I split off from this PR and incorporated #5979 in another branch, but the failing test still didn't pass.

@willemarcel
Copy link
Contributor

@HelNershingThapa @ramyaragupathy When I commented out this line, the time spent running the test decreased from 599 to 119ms. So Maybe it could be a good solution, as you're testing if the user can see the project or not.

@HelNershingThapa
Copy link
Contributor

Indeed @willemarcel, commenting out that particular line does improve the test execution time significantly and allows the test case to pass. It makes sense since the line checks the visibility of a comment in the "questions and comments" section.

After implementing this workaround, CircleCI encountered another similar issue. Another test case is now failing due to the same "timeout exceeded" problem (more info here) 🤯.

@willemarcel
Copy link
Contributor

@HelNershingThapa
Copy link
Contributor

@willemarcel looks like so; it's using expect without an assertion. Thank you for catching that! I'll revise it and remove the expect function to see if it resolves the problem.

@sonarcloud
Copy link

sonarcloud bot commented Oct 16, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 3 Security Hotspots
Code Smell A 14 Code Smells

No Coverage information No Coverage information
0.2% 0.2% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

Copy link

sonarcloud bot commented Nov 30, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 1 Security Hotspot
Code Smell A 28 Code Smells

No Coverage information No Coverage information
0.2% 0.2% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@tsmock
Copy link
Contributor Author

tsmock commented Nov 30, 2023

I just ran a bunch of tests on the develop branch and this branch.

The develop branch tests were run on a mac (100% pass rate) and on a linux box (~50% pass rate). The pass rate for the chartjs branch was a bit worse on the linux box (~33%).

The most common tests to "fail" were in src/views/tests/taskSelection.test.js (~66%), but I suspect that the failing tests are generally flaky. I'm going to run a while loop overnight, but I suspect that all of the tests that failed in the chartjs branch will also fail in the develop branch, just at lower rates.

@tsmock
Copy link
Contributor Author

tsmock commented Dec 6, 2023

Logs from running for several days (linux)
develop-tasks.logs.tar.gz
develop-mac-tasks.logs.tar.gz
chartjs-tasks.logs.tar.gz

@tsmock
Copy link
Contributor Author

tsmock commented Dec 6, 2023

I've looked for failing test files from the logs in the previous comment. Both of the linux runs were on the same box, run at the same time. I'll note that there are some cancellations counted in all environments (ctrl+c) due to OOM issues.

Failing test files
File chartjs (linux, 394 runs) develop (linux, 385 runs) develop (mac, 1029 runs)
src/components/banner/tests/topBanner.test.js 1
src/components/contributions/tests/myProjectsDropdown.test.js 1
src/components/header/tests/notificationBell.test.js 1
src/components/homepage/tests/stats.test.js 1
src/components/notifications/tests/notificationBodyCard.test.js 1
src/components/projectDetail/tests/questionsAndComments.test.js 2
src/components/projectDetail/tests/similarProjects.test.js 1 1
src/components/projects/tests/moreFiltersForm.test.js 2
src/components/taskSelection/tests/contributions.test.js 6 5
src/components/taskSelection/tests/action.test.js 3
src/components/taskSelection/tests/actionSidebars.test.js 1
src/components/taskSelection/tests/contributions.test.js 6
src/components/taskSelection/tests/index.test.js 9
src/components/taskSelection/tests/lockedTasks.test.js 2 1
src/components/taskSelection/tests/resourcesTab.test.js 1
src/components/taskSelection/tests/taskList.test.js 5 5 14
src/components/teamsAndOrgs/tests/members.test.js 1
src/views/tests/management.test.js 2 3
src/views/tests/organisationManagement.test.js 4
src/views/tests/project.test.js 1 2
src/views/tests/taskAction.test.js 4
src/views/tests/taskSelection.test.js 33 27 30
src/views/tests/teams.test.js 5
src/views/tests/userDetail.test.js 5
src/views/tests/users.test.js 4 1
Totals 57 40 94

The absolute flakiest test file is src/views/tests/taskSelection.test.js.

@tsmock tsmock force-pushed the fixup/chartjs-warnings branch 2 times, most recently from 81025b3 to fabd78a Compare February 7, 2024 18:18
@github-actions github-actions bot added dependencies Pull requests that update a dependency file javascript labels Feb 7, 2024
@github-actions github-actions bot removed dependencies Pull requests that update a dependency file javascript labels Feb 8, 2024
@tsmock tsmock force-pushed the fixup/chartjs-warnings branch 3 times, most recently from d9a2d98 to 90f5cbf Compare February 8, 2024 18:45
@tsmock
Copy link
Contributor Author

tsmock commented Feb 8, 2024

OK. It has taken me awhile, but I've figured out what is causing the test failures.

React.lazy.

In order to understand why React.lazy is a problem, we must first understand how jest starts and times tests.
Specifically, jest will load all files required (so a file importing foo importing bar will load the file + foo + bar before starting the tests). I don't know why it does the imports first, but the effect of doing so is that it means that the time of loading the imports is used against the tests.
By contrast, imports loaded via React.lazy are loaded from disk dynamically, and are counted against the time for the test.

In other words,

  1. jest loads test file
  2. jest recursively resolves imports for the test file
  3. jest starts tests
  4. For each test
    1. Start a timer
    2. Run the test
      • If React.lazy is used somewhere, the additional imports required are resolved as the code executes.
    3. Stop the timer, report if the test took too long

So, how can we work around this import timing problem?

  1. We can import the slow module in the test itself -- in this case, we must document why we have an unused import.
  2. We can mock the slow module

Of the two problems, (1) is probably better, since it should be more reliable (AKA: we don't have to keep mocks in sync with code).

@tsmock tsmock force-pushed the fixup/chartjs-warnings branch 2 times, most recently from 0ef6620 to 13a51c4 Compare February 8, 2024 19:02
Copy link

sonarcloud bot commented Feb 12, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@manjitapandey
Copy link

The graph on manage statistics page has differ from previous. The graphs previously shows adjacent individual pillars for tasks mapped and validated. However, the new one shows both mapped and validated tasks on single pillar.

image
cc: @ramyaragupathy , @royallsilwallz

@tsmock
Copy link
Contributor Author

tsmock commented Feb 14, 2024

It looks like this was a bug; the original definitions (see #4103) for the graphs were

    scales: {
      yAxes: [
        {
          stacked: true,
          ticks: {
            beginAtZero: true,
          },
        },
      ],
      xAxes: [
        {
          stacked: true,
        },
      ],
    },

If we want to keep the second style (side-by-side), we need to drop stacked: true from the axis definitions in tasksStatsChart.js.

@ramyaragupathy ramyaragupathy merged commit 7307769 into hotosm:develop Feb 20, 2024
6 checks passed
@tsmock tsmock deleted the fixup/chartjs-warnings branch February 20, 2024 12:21
@ramyaragupathy ramyaragupathy added this to the v4.7.1 milestone Mar 21, 2024
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.

5 participants