Skip to content

Enable parallelism #92

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

Merged
merged 21 commits into from
Mar 21, 2024
Merged

Enable parallelism #92

merged 21 commits into from
Mar 21, 2024

Conversation

jnooree
Copy link
Contributor

@jnooree jnooree commented Mar 20, 2024

Closes #74. I also reviewed #73.

jnooree added a commit to jnooree/cpp-linter-action that referenced this pull request Mar 20, 2024
Copy link
Contributor

@2bndy5 2bndy5 left a comment

Choose a reason for hiding this comment

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

This was a pleasant surprise! Thanks for taking this on.

@2bndy5
Copy link
Contributor

2bndy5 commented Mar 20, 2024

🎉 Benchmarking at repo root on my intel i7-9700 CPU

hyperfine "cpp-linter -l false -a false -j 1" "cpp-linter -l false -a false -j 0"
Benchmark 1: cpp-linter -l false -a false -j 1
  Time (mean ± σ):     51.437 s ±  2.264 s    [User: 25.653 s, System: 12.871 s]
  Range (min … max):   49.109 s … 54.378 s    10 runs

Benchmark 2: cpp-linter -l false -a false -j 0
  Time (mean ± σ):     26.391 s ±  0.498 s    [User: 26.286 s, System: 12.340 s]
  Range (min … max):   25.500 s … 27.063 s    10 runs

Summary
  cpp-linter -l false -a false -j 0 ran
    1.95 ± 0.09 times faster than cpp-linter -l false -a false -j 1

Copy link
Contributor

@2bndy5 2bndy5 left a comment

Choose a reason for hiding this comment

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

I'm also noticing that you don't use a word delimiter _ in your variable names (which should be snake-cased according to PEP8). This is throwing my spellchecker in a frenzy.

@jnooree
Copy link
Contributor Author

jnooree commented Mar 20, 2024

I'm also noticing that you don't use a word delimiter _ in your variable names (which should be snake-cased according to PEP8). This is throwing my spellchecker in a frenzy.

If you want to change some names, please locate the names so I could update them.

2bndy5

This comment was marked as outdated.

@jnooree
Copy link
Contributor Author

jnooree commented Mar 20, 2024

It is odd to request a refactor from the reviewer. Maybe I didn't describe what I wanted (in accordance with PEP8).

Thanks for the suggestions! I don't use a spell checker, so I just wanted to know the spell checker output of your environment.

@2bndy5
Copy link
Contributor

2bndy5 commented Mar 21, 2024

I'm adding a pre-commit hook to run codespell in #93. I realized my request wasn't integrated into our CI checks. ruff is pretty relaxed about variable naming in a function's scope.

The actual spell checker that I'm using is a VSCode extension (not the codespell package).

Co-authored-by: Brendan <2bndy5@gmail.com>
@jnooree
Copy link
Contributor Author

jnooree commented Mar 21, 2024

I just finished variable renaming! No more force pushes.

Copy link

codecov bot commented Mar 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (e33a5e6) to head (a4abbe3).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #92   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           20        20           
  Lines         1392      1437   +45     
=========================================
+ Hits          1392      1437   +45     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

2bndy5

This comment was marked as resolved.

@jnooree
Copy link
Contributor Author

jnooree commented Mar 21, 2024

I think there are some suspicious missing lines in the coverage report. I will investigate them first.

@jnooree
Copy link
Contributor Author

jnooree commented Mar 21, 2024

The "missing" lines are actually covered in tests (checked manually), but coverage could not track the changes. Are we ok to skip them or need some workarounds?

Co-authored-by: Brendan <2bndy5@gmail.com>
@2bndy5
Copy link
Contributor

2bndy5 commented Mar 21, 2024

Well, I'm still puzzled why only the lines_changed_only arg isn't triggered in coverage.

I'd rather not skip the lines just to make coverage report "look" better. I prefer we actually have reproducible results to measure for code coverage.

@2bndy5 2bndy5 dismissed their stale review March 21, 2024 03:02

resolved

@jnooree
Copy link
Contributor Author

jnooree commented Mar 21, 2024

I'll experiment for other methods to get coverage report correctly and share if any succeded. I think this looks like an upstream bug tho.

@2bndy5
Copy link
Contributor

2bndy5 commented Mar 21, 2024

I'm seeing a lot of .coverage files created in the tests folder. When I run coverage combine, these tests/.coverage.* files are not combined into the report. I think one of the tests is changing coverage's working directory.

@2bndy5
Copy link
Contributor

2bndy5 commented Mar 21, 2024

If I move all the rogue .coverage files into repo root and run coverage combine, then the output says it skips 146 "duplicate data"; only 6 .coverage files are actually combined.

This skipping "duplicate data" isn't a new thing, but spawning the coverage files in the tests folder is a new thing. I hesitate to call it a problem because I don't know the inner-workings of the coverage package.

None of this actually changed the overall code coverage reported.

@jnooree
Copy link
Contributor Author

jnooree commented Mar 21, 2024

Yes, I also just found out that the monkeypatch might cause problems for subprocesses. Fixed in 696d83c.

@2bndy5
Copy link
Contributor

2bndy5 commented Mar 21, 2024

The rogue .coverage file are were spawned when running

  • tests/capture_tools_output/test_database_path.py

@jnooree
Copy link
Contributor Author

jnooree commented Mar 21, 2024

Oh, ok. It seems the tests are not very pleased with out-of-order logs. I'll update them shortly.

@2bndy5
Copy link
Contributor

2bndy5 commented Mar 21, 2024

I must say, you are picking up on the cause of these errors faster than I'm accustomed to. Its very refreshing!

Copy link
Contributor

@2bndy5 2bndy5 left a comment

Choose a reason for hiding this comment

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

Oh, ok. It seems the tests are not very pleased with out-of-order logs

That was it completely! coverage is restored and new functionality verified 💯

@2bndy5 2bndy5 merged commit ab3e45a into cpp-linter:main Mar 21, 2024
98 checks passed
@2bndy5
Copy link
Contributor

2bndy5 commented Mar 21, 2024

@jnooree Thank you soooo much!❤️ I don't think I would've been able to solve #74 on my own.

@jnooree
Copy link
Contributor Author

jnooree commented Mar 21, 2024

I should thank you and the maintainers first for creating such a great package/workflow. Great work!

jnooree added a commit to jnooree/cpp-linter-action that referenced this pull request Mar 21, 2024
@2bndy5 2bndy5 added enhancement New feature or request minor A minor version bump labels Mar 26, 2024
2bndy5 pushed a commit to jnooree/cpp-linter-action that referenced this pull request Mar 28, 2024
2bndy5 added a commit to cpp-linter/cpp-linter-action that referenced this pull request Mar 28, 2024
* feat: add --jobs parameter to action
  See cpp-linter/cpp-linter#92 for the related CLI updates.
* adjustments for docs
---------

Co-authored-by: Brendan <2bndy5@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request minor A minor version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parallelize lint checks
2 participants