8000 Enable tox --parallel by graingert · Pull Request #207 · twisted/ldaptor · GitHub
[go: up one dir, main page]
Skip to content

Enable tox --parallel #207

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 19 commits into from
Oct 5, 2020
Merged

Enable tox --parallel #207

merged 19 commits into from
Oct 5, 2020

Conversation

graingert
Copy link
Member
@graingert graingert commented Oct 4, 2020 •
edited
Loading

Contributor Checklist:

  • I have updated the release notes at docs/source/NEWS.rst
  • I have updated the automated tests.
  • All tests pass on your local dev environment. See CONTRIBUTING.rst.

@graingert
Copy link
Member Author

I don't expect this to be useful without a GHA update, https://github.community/t/log-group-multiplexing/130543/5

@codecov
Copy link
codecov bot commented Oct 4, 2020 •
edited
Loading

Codecov Report

Merging #207 into master will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #207      +/-   ##
==========================================
- Coverage   83.30%   83.30%   -0.01%     
==========================================
  Files          87       87              
  Lines       11579    11570       -9     
  Branches     1184     1184              
==========================================
- Hits         9646     9638       -8     
  Misses       1815     1815              
+ Partials      118      117       -1
Impacted Files Coverage Δ
ldaptor/protocols/ldap/ldapconnector.py 42.85% <0.00%> (-0.68%) ⬇️
ldaptor/ldiftree.py 89.56% <0.00%> (-0.08%) ⬇️
ldaptor/protocols/ldap/ldapserver.py 81.84% <0.00%> (-0.06%) ⬇️
ldaptor/protocols/ldap/ldapclient.py 91.66% <0.00%> (-0.06%) ⬇️
ldaptor/inmemory.py 96.87% <0.00%> (-0.03%) ⬇️
ldaptor/schema.py 96.72% <0.00%> (-0.01%) ⬇️
ldaptor/checkers.py 0.00% <0.00%> (ø)
ldaptor/test/test_server.py 100.00% <0.00%> (ø)
ldaptor/protocols/ldap/ldifdelta.py 90.74% <0.00%> (+0.83%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a5446e...19f2b89. Read the comment docs.

@graingert
demo multiplexed build log
a46fdce
@graingert
actually run things concurrently
056000e 
GHA machines only have 1 cpu - so force parallelism
@graingert graingert changed the title Enable tox --parallel --parallel-live to demo multiplexed github build output Enable tox --parallel Oct 4, 2020
@graingert graingert marked this pull request as ready for review October 4, 2020 16:30
@graingert graingert requested a review from adiroiban October 4, 2020 16:30
@graingert
Copy link
Member Author

@adiroiban this might be nicer, what do you think? Currently we have to wait for the job to finish before seeing any output - https://github.community/t/log-group-multiplexing/130543/5 but maybe that's not so bad?

graingert
graingert commented Oct 4, 2020
View reviewed changes
.github/workflows/main.yml
if: startsWith(matrix.os, 'windows-') || startsWith(matrix.os, 'macOS-')
run: echo '::set-env name=TOX_SKIP_ENV::pypy3.*'

- name: build coverage for pypy3 # coverage doesn't like being built in parallel
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
- name: build coverage for pypy3 # coverage doesn't like being built in parallel
# coverage doesn't like being built in parallel, see https://git.io/JUdlT
- name: build coverage for pypy3

@graingert
Copy link
Member Author

Takes about a minute longer overall, and the coverage change looks worrying

adiroiban
adiroiban reviewed Oct 4, 2020
View reviewed changes
Copy link
Member
@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

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

I think tox --parallel is ok for local builds.

For automated tests, I don't know if this help. I think that the VMs are 2 cores.... and a lot of tests are CPU intensive.

And is best if we see each environment as a separate status.

The coverage config change is ok.

I would say that is best to keep non-parallel runs for GHA runs.

Thanks!

@graingert
Copy link
Member Author

For automated tests, I don't know if this help.

Could do a run without --parallel I think mostly we save time on the pip installs

graingert
graingert commented Oct 4, 2020
View reviewed changes
@graingert
Copy link
Member Author

@adiroiban tox without --parallel is quite a bit slower

adiroiban
adiroiban approved these changes Oct 4, 2020
View reviewed changes
Copy link
Member
@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

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

Thanks for the test ;)

What I wanted to say, is that for GitHub Action we can have separate jobs for each environment.

But I am ok to run one job for each OS, and inside that job to test various Python version.

You can merge it as you think is best.

Thanks!

@graingert
Copy link
Member Author
graingert commented Oct 4, 2020 •
edited
Loading

@adiroiban hmm the coverage has gone down only on while True: lines that are clearly covered... I think this might be a coverage combine bug

@graingert graingert mentioned this pull request Oct 4, 2020
Closed
@graingert
Copy link
Member Author

@adiroiban nedbat/coveragepy#1036

@adiroiban
Copy link
Member

Thanks. I think that this can be merged.

@graingert graingert merged commit c687081 into twisted:master Oct 5, 2020
hugovk pushed a commit to hugovk/html5lib-python that referenced this pull request May 2, 2021
@jayaddison @hugovk
Enable stdout output from tox in parallel mode
4da68a9 
With credit to @graingert via twisted/ldaptor#207
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adiroiban adiroiban adiroiban approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@graingert @adiroiban
0