Skip to content

Project-Euler/test/Problem044.test.js runs for ~ 17 minutes #1193

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

Closed
defaude opened this issue Oct 15, 2022 · 9 comments · Fixed by #1393
Closed

Project-Euler/test/Problem044.test.js runs for ~ 17 minutes #1193

defaude opened this issue Oct 15, 2022 · 9 comments · Fixed by #1393
Labels
help wanted Needs to be worked on performance Performance improvement tests Adds or fixes tests; issue that points out bugs in the tests

Comments

@defaude
Copy link
Contributor

defaude commented Oct 15, 2022

I just stumbled upon this gem while executing the tests locally and then wondering what's going on.

See https://github.com/TheAlgorithms/JavaScript/actions/runs/3255189818/jobs/5344248296#step:5:161

@raklaptudirm what do you think? Should we

  1. disable the test so it's skipped and doesn't clog up our CI
  2. figure out why this immense delay happens, fix the test (or the implementation) and re-enable the test once we're good
@defaude
Copy link
Contributor Author

defaude commented Oct 15, 2022

It's this one:

  // Project Euler Second Value for Condition Check
  test('if the number is greater or equal to 2167', () => {
    expect(problem44(2167)).toBe(8476206790)
  })

I guess it's because this is running into really, really high numbers, maybe? Neither the CPU nor the RAM usage goes up substantially (at least on my machine) but for some reason, this takes ages to complete.

@defaude
Copy link
Contributor Author

defaude commented Oct 15, 2022

I just realized this is even more severe as assumed initially: Since we're running the test in a pre-commit hook, the simple act of just committing changes locally becomes a minute-long ordeal...

@appgurueu
Copy link
Collaborator

I just realized this is even more severe as assumed initially: Since we're running the test in a pre-commit hook, the simple act of just committing changes locally becomes a minute-long ordeal...

OOF. Ideally CI & hooks should only run tests on the files that were changed.

@raklaptudirm
Copy link
Member

Jest usually caches tests so the precommit hook should not be an issue.

@raklaptudirm raklaptudirm added help wanted Needs to be worked on tests Adds or fixes tests; issue that points out bugs in the tests performance Performance improvement labels Oct 16, 2022
@utkarsh-shrivastav77
Copy link

Is this issue still open to contribute

@appgurueu
Copy link
Collaborator

Yes, feel free to optimize the solution to problem 44.

@utkarsh-shrivastav77
Copy link

utkarsh-shrivastav77 commented May 12, 2023

export { problem44 } instead of this can I use console.log for the answer

@appgurueu
Copy link
Collaborator

export { problem44 } instead of this can I use console.log for the answer

No, that doesn't improve the performance at all and just harms code quality.

@defaude
Copy link
Contributor Author

defaude commented Oct 1, 2023

While we changed the process so that the pipeline for pull requests only executes changed test files, the issue with the super-long-running test execution. This hinders not only the "regular" pipeline regarding runtime: It also kind of deters people from just running the tests locally as it takes forever. As a result, we have three failing tests right now :)

=> I took the liberty to just .skip the long-running test. This way, we still have the test case for those who want to play around with it and for documentation's sake. But we don't run it every single time.

raklaptudirm pushed a commit that referenced this issue Oct 2, 2023
* test: skip test that's running way too long

It's good to have the test there, but there's no use having it running for ~30 minutes or so in the GitHub Action

close #1193

* Updated Documentation in README.md

---------

Co-authored-by: github-actions <${GITHUB_ACTOR}@users.noreply.github.com>
raklaptudirm pushed a commit that referenced this issue Oct 3, 2023
…#1407)

* chore: Switch to Node 20 + Vitest

* chore: migrate to vitest mock functions

* chore: code style (switch to prettier)

* test: re-enable long-running test

Seems the switch to Node 20 and Vitest has vastly improved the code's and / or the test's runtime!

see #1193

* chore: code style

* chore: fix failing tests

* Updated Documentation in README.md

* Update contribution guidelines to state usage of Prettier

* fix: set prettier printWidth back to 80

* chore: apply updated code style automatically

* fix: set prettier line endings to lf again

* chore: apply updated code style automatically

---------

Co-authored-by: github-actions <${GITHUB_ACTOR}@users.noreply.github.com>
Co-authored-by: Lars Müller <34514239+appgurueu@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Needs to be worked on performance Performance improvement tests Adds or fixes tests; issue that points out bugs in the tests
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants