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

Better use of golden files in CI #724

Merged
merged 4 commits into from
Apr 17, 2024
Merged

Better use of golden files in CI #724

merged 4 commits into from
Apr 17, 2024

Conversation

smelc
Copy link
Contributor

@smelc smelc commented Apr 16, 2024

Changelog

- description: |
    Better use of golden files in CI
# uncomment types applicable to the change:
  type:
  # - feature        # introduces a new feature
  # - breaking       # the API has changed in a breaking way
  # - compatible     # the API has changed but is non-breaking
  # - optimisation   # measurable performance improvements
  # - improvement    # QoL changes e.g. refactoring
  # - bugfix         # fixes a defect
  - test           # fixes/modifies tests
  - maintenance    # not directly related to the code
  # - release        # related to a new release preparation
  # - documentation  # change in code docs, haddocks...

Context

  • This PR avoids an extra testing step (by checking golden files for modifications)
  • This PR reports golden files that are useless (because they didn't get generated)

How to trust this PR

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • Self-reviewed the diff

Base automatically changed from smelc/update-cardano-api-to-8.45.0.0 to main April 16, 2024 14:56
@smelc smelc force-pushed the smelc/improve-golden-ci-pr branch 2 times, most recently from 16a99a5 to 90b97af Compare April 16, 2024 14:58
@smelc smelc marked this pull request as ready for review April 16, 2024 15:01
@smelc smelc requested review from a team as code owners April 16, 2024 15:01
Copy link
Contributor

@palas palas left a comment

Choose a reason for hiding this comment

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

Nice improvement. I also tried the scripts manually and they work. My two cents are two suggestions about presentation.

.github/workflows/haskell.yml Outdated Show resolved Hide resolved
Comment on lines 165 to 167
echo "💣 The following golden files are not up-to-date:"
git ls-files -m
echo "Please run the tests locally and update the golden files, or fix your changes."
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
echo "💣 The following golden files are not up-to-date:"
git ls-files -m
echo "Please run the tests locally and update the golden files, or fix your changes."
echo "💣 The following golden files are not up-to-date:"
echo -e
git ls-files -m
echo -e
echo "These are the changes that were made:"
echo -e
git diff
echo -e
echo "Please run the tests locally and update the golden files, or fix your changes."

In addition to the previous suggestion, it may be useful to display the diff. That way you can see what the differences look like. So, for example, if it is a whitespace change, it may be obvious you are missing pretty-printing them. Or if a particular key in the JSON is wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I rather keep the output of the CI short, so I will skip this one 👍

@newhoggy newhoggy self-requested a review April 17, 2024 11:32
@newhoggy
Copy link
Contributor

Not sure why all the Windows CI fails.

echo "⚠️ The following golden files are not used anymore:"
git ls-files -d
echo "Please delete them."
exit 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I simply disabled the new checks on Windows: ad0b99a

@smelc smelc force-pushed the smelc/improve-golden-ci-pr branch from 6547510 to 94de866 Compare April 17, 2024 12:05
@smelc smelc force-pushed the smelc/improve-golden-ci-pr branch from 94de866 to d90f4ce Compare April 17, 2024 12:06
@smelc smelc force-pushed the smelc/improve-golden-ci-pr branch from 7d508c9 to ad0b99a Compare April 17, 2024 12:19
@smelc smelc enabled auto-merge April 17, 2024 12:20
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

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

Nice 👍

@smelc smelc added this pull request to the merge queue Apr 17, 2024
Merged via the queue into main with commit 6c57ef3 Apr 17, 2024
16 checks passed
@smelc smelc deleted the smelc/improve-golden-ci-pr branch April 17, 2024 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants