Skip to content

Support test sets for printer options#132

Closed
gustavoavena wants to merge 1 commit intofourmolu:masterfrom
gustavoavena:refactor_printer_spec
Closed

Support test sets for printer options#132
gustavoavena wants to merge 1 commit intofourmolu:masterfrom
gustavoavena:refactor_printer_spec

Conversation

@gustavoavena
Copy link
Contributor

What

Refactor the PrinterSpec module to support Test Sets.
Each set will test an instance of PrinterOpts, i.e. a combination of fourmolu's customizations.

How

Define data types to represent a test case and a test set:

  • A test case represents an input and the expected output.
  • A test set contains:
    • An instance of PrinterOpts, which have the combination of options you want to test.
    • A label, e.g. "ormolu" for ormolu's default option.
    • A list of test cases.

This decreases the coupling between the code that defines the configs, examples and file list from the code that runs and checks the examples.

Why

See #49. Fourmolu now has a lot of options, and we need an easier way to test them properly.

With this change, testing a new combination of options should also be much easier.

I don't think this should necessarily be the final state, but IMO it's a step in the right direction.

See an example of adding a test suite for a new option in #102.

RFC and Possible Follow Ups

  • I added a canBreakIdempotence boolean to the test set data type, because some of the customizations currently under review can do this (e.g. Automatic line breaking at specific limit #102, Do hang record constructors when turned on setting #103).

    • This is to avoid adding conditions for skipping the idempotence check inside checkExample, which shouldn't have any logic specific to an option.
  • We could decouple things even more?

    • Some ideas:
      • Create a separate module for the test set configs and/or new data types.
      • Create separate Specs for each config.
      • Create subfolders for output files, instead of using suffixes.
      • This could make reusing files from other options a bit more confusing, but it would improve the examples directory structure.
    • Since we don't have that many test sets now, I don't think that working on these things is necessary, but I would love to get your thoughts about this.

@gustavoavena gustavoavena mentioned this pull request Nov 19, 2021

Run `cabal test` and `./format.sh` before submitting any pull requests.

If you're adding a new customization, please add a new test set for it in the `PrinterSpec` module.
Copy link

Choose a reason for hiding this comment

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

BTW, with #129 in place this would be mandatory as it derives test cases from the option type.

maintainer:
Matt Parsons <parsonsmatt@gmail.com>
George Thomas <georgefsthomas@gmail.com>

Copy link
Collaborator

Choose a reason for hiding this comment

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

Per DEVELOPER.md, can we avoid making formatting changes? To reduce merge conflicts with ormolu

@brandonchinn178
Copy link
Collaborator

I really like the overall thrust of this PR! However, I'm personally particularly sensitive to changes that would cause bad merge conflicts when merging Ormolu changes from upstream. @gustavoavena what do you think about keeping PrinterSpec.hs as-is (or even better, reverting it completely to the way Ormolu has it in their codebase, as much as possible), and then writing a separate test suite to test options? I'd even be ok reusing the current infrastructure of *.hs/*-out.hs/*-four-out.hs files (although personally I find it a bit hard to browse the test cases with this format), but just having a separate test suite would allow us to have total control over the Fourmolu-specific configuration we want to test. Thoughts?

@brandonchinn178
Copy link
Collaborator

I added an update to #49. To reiterate what I said in that issue, many thanks for the work you've put into this PR. In light of the new direction, though, I'll be closing this PR for now. Please let me know if you have any questions, comments, or concerns. Thanks!

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.

3 participants