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

chore(sequencer): don't generate tests using macros #1726

Closed
wants to merge 2 commits into from

Conversation

ethanoroshiba
Copy link
Contributor

@ethanoroshiba ethanoroshiba commented Oct 23, 2024

Summary

Replaced macros generating fee action unit tests by expanding them.

Background

Macro-generated tests would be a headache to debug, since failures may point to lines of code which don't exist. While there is significantly more code, I think it's preferable to aide in future debugging.

Changes

  • Removed macro which generated FeeChange unit tests.
  • Created helper functions for shortening fee change tests as much as possible.
  • Created fee change tests for every possible variant of FeeChange (i.e. one for each action).

Testing

Passing all tests.

Related Issues

closes #1715

@github-actions github-actions bot added the sequencer pertaining to the astria-sequencer crate label Oct 23, 2024
@ethanoroshiba ethanoroshiba marked this pull request as ready for review October 23, 2024 18:27
@ethanoroshiba ethanoroshiba requested a review from a team as a code owner October 23, 2024 18:27
@ethanoroshiba ethanoroshiba changed the title chore(sequencer): improve FeeChange testing chore(sequencer): don't generate tests using macros Oct 23, 2024
Copy link
Contributor

@Fraser999 Fraser999 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 afraid I see this PR as a step in the wrong direction.

While I agree that using macros can lead to somewhat hard-to-debug cases, I think the benefits here far outweigh that minor drawback. Debugging wouldn't be particularly hard (the generated tests are relatively simple), whereas getting all the copy-pasted code correct is much harder, for authors, maintainers and reviewers.

I haven't given the changes a proper review, but skimming has revealed a few such errors, lending weight to the case for using a macro here. In the PR which introduced the macro, I made several such mistakes in the highly repetitive, non-macro fee code, and the macro tests caught these. The macro tests were in fact introduced because I noticed that the existing unit test for fee change was only covering a few of the fee variants.

My strong preference would be to close this PR unmerged. If we don't reach agreement on that, then I'll do a full review at that point.

@Fraser999 Fraser999 deleted the ENG-944/improve_fee_change_test branch November 19, 2024 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sequencer pertaining to the astria-sequencer crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sequencer: make fee change testing easier to debug
2 participants