Skip to content

fix: Deal with different group orders#6394

Merged
rjsparks merged 5 commits intoietf-tools:mainfrom
larseggert:fix-6393
Sep 29, 2023
Merged

fix: Deal with different group orders#6394
rjsparks merged 5 commits intoietf-tools:mainfrom
larseggert:fix-6393

Conversation

@larseggert
Copy link
Copy Markdown
Collaborator

Fixes #6393

@rjsparks
Copy link
Copy Markdown
Member

rjsparks commented Sep 27, 2023

Unless I'm missing something this change cannot have fixed the bug. The change may be harmless, but if it actually does something, there's something more broken in the factories to address.

The bare calls to GroupFactory make names this way:
acronym = factory.Sequence(lambda n: 'acronym%d' %n)

So that call would have made acronym1, acronym2, and acronym3.

@larseggert
Copy link
Copy Markdown
Collaborator Author

Hm. I wasn't actually able to reproduce the bug locally. Let me run the test for a few more hours to see if it kicks in.

@larseggert larseggert marked this pull request as draft September 27, 2023 13:27
@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 27, 2023

Codecov Report

Merging #6394 (7398ea0) into main (61045d3) will increase coverage by 0.00%.
Report is 2 commits behind head on main.
The diff coverage is 72.22%.

@@           Coverage Diff           @@
##             main    #6394   +/-   ##
=======================================
  Coverage   88.68%   88.69%           
=======================================
  Files         290      290           
  Lines       40421    40431   +10     
=======================================
+ Hits        35848    35860   +12     
+ Misses       4573     4571    -2     
Files Coverage Δ
ietf/doc/views_review.py 95.16% <100.00%> (ø)
ietf/nomcom/views.py 93.01% <100.00%> (+0.36%) ⬆️
ietf/review/utils.py 92.13% <75.00%> (+0.03%) ⬆️
ietf/review/policies.py 97.47% <50.00%> (-1.43%) ⬇️

... and 2 files with indirect coverage changes

@jennifer-richards jennifer-richards dismissed their stale review September 27, 2023 13:48

Convinced by @rjsparks comment that there must be something else going on

@larseggert larseggert changed the title fix: Make sure groups in test have different acronyms fix: Deal with different group orders Sep 28, 2023
@larseggert larseggert marked this pull request as ready for review September 28, 2023 08:29
@rjsparks
Copy link
Copy Markdown
Member

We'll merge this. It might have been better to change what the sreq app does so that the output is deterministic, but the investment would be better made on replacing the sreq app.

@rjsparks rjsparks merged commit 67872f4 into ietf-tools:main Sep 29, 2023
@larseggert larseggert deleted the fix-6393 branch September 29, 2023 14:30
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Oct 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CI failure: ietf.secr.sreq.tests.SessionRequestTestCase.test_edit

3 participants