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: gitdist: dist-repo-status: Display tag or SHA1 instead of 'HEAD' #590

Merged
merged 5 commits into from
Jun 30, 2023

Conversation

bartlettroscoe
Copy link
Member

@bartlettroscoe bartlettroscoe commented Jun 30, 2023

There was a bug is the previous PR #587. See commit messages for details.

This replaces the following PR that seems to show a GitHub defect described in #589 (comment):

I was afraid that the GitHub defect would cause the merge of the wrong commits.

These were exposed with Python 3.6.8.  These changes make all of the warnings
go away for the gitdist unit tests.
This makes the tests a little more maintainable.
I broke the system-level functioning of detailed head info when I refactored
and then squashed the commit:

  gitdist: dist-repo-status: Display tag or SHA1 instead of 'HEAD'

to change how 'showMoreHeadDetails' is passed in to get the CheckinTest.py
tests to pass.  I did not go back and ensure that the 'gitdist' script was
behaving as it should.  I was missing a system-level test to ensure the
correct behavior.

The fix is one-line (which I will add in the next commit) but it took over 100
lines of test code (in this commit) to test this change.  That is terrible.
The tests need to be more maintainable.  I added a todo in the unit tests file
to refactor the testing code to reduce duplication and make the tests more
maintainable.  The way the testing is done now is technical debt and is
starting to look like the CheckinTest.py tests (which is why it is so hard to
modify that code).  I really should have done that refactroing here but it
should be done before any other major work on this code.
I broke the system-level functioning of the detached head state details when I
refactored and then squashed the commit:

  gitdist: dist-repo-status: Display tag or SHA1 instead of 'HEAD'

to change how 'showMoreHeadDetails' is passed in to get the CheckinTest.py
tests to pass.  I did not go back and ensure that the 'gitdist' script was
behaving as it should.  I was missing a system-level test to ensure the
correct behavior.

This fix makes the failing test added in the previous commit pass.
Copy link
Collaborator

@rabartlett1972 rabartlett1972 left a comment

Choose a reason for hiding this comment

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

I will let this PR merge without an external review.

@bartlettroscoe bartlettroscoe enabled auto-merge June 30, 2023 18:44
@bartlettroscoe bartlettroscoe merged commit f6e0838 into master Jun 30, 2023
bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this pull request Sep 23, 2023
…bits

This brings in the following TriBITS PRs:

* TriBITSPub/TriBITS#591: Allow 100% raw CMake to be used in a
  TriBITS-compliant package (TriBITSPub/TriBITS#582)

* TriBITSPub/TriBITS#588: gitdist: Pass in '-c color.status=always' when
  --dist-no-color is not added

* TriBITSPub/TriBITS#590: Fix: gitdist: dist-repo-status: Display tag or SHA1
  instead of 'HEAD'

* TriBITSPub/TriBITS#587: gitdist: dist-repo-status: Display tag or SHA1
  instead of 'HEAD'

* TriBITSPub/TriBITS#586: More cleanup of packaging support
  (trilinos#11976)

This goes back to TriBITS commits from June 2023.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants