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

Eval plugin: support ghc 9.0.1 #1997

Merged
merged 12 commits into from
Jul 5, 2021
Merged

Conversation

berberman
Copy link
Collaborator

Tests are broken due to:

These are minor differences, but I'm not sure if it's a good idea to have separate golden test files for GHC 9...

@pepeiborra
Copy link
Collaborator

Awesome!

Re the test files, I think that we will need to update them for 9.0 and disable the eval plugin tests for 8.x to avoid maintaining two sets of golden tests files.

@jneira
Copy link
Member

jneira commented Jul 3, 2021

We are gonna support ghc 8 for a while, so I would wait to a stable and widely used ghc 9 to remove tests for older versions
The ghc golden tests file will not need maintenance effort, as they are not gonna change (probably) and we could only add new tests for 9.

@pepeiborra
Copy link
Collaborator

We are gonna support ghc 8 for a while, so I would wait to a stable and widely used ghc 9 to remove tests for older versions
The ghc golden tests file will not need maintenance effort, as they are not gonna change (probably) and we could only add new tests for 9.

Removing the tests for ghc 8.x does not mean that we don't support the plugin in that version, but simply that we only test it with ghc 9.

Either now or in the future we are going to need to migrate the test suite to ghc 9. My view is that it's better to do it now that it's cheap rather than wait until later when it's more expensive.

@jneira
Copy link
Member

jneira commented Jul 4, 2021

Removing the tests for ghc 8.x does not mean that we don't support the plugin in that version, but simply that we only test it with ghc 9.

Sure but new bugs could be introduced and in that case, depending on their severity, it will be worse than remove support for the plugin

Either now or in the future we are going to need to migrate the test suite to ghc 9. My view is that it's better to do it now that it's cheap rather than wait until later when it's more expensive.

Mmm we will have to

  • copy golden tests and change them and
  • add and cpp (or runtime) if for load files from one location or another

When we will drop support for ghc <= 8, we will have to remove old golden files and the if (like the rest of dozens of similar ifs everywhere).
The extra effort (create the if now and remove later) worths it imo.

@pepeiborra
Copy link
Collaborator

pepeiborra commented Jul 4, 2021

Removing the tests for ghc 8.x does not mean that we don't support the plugin in that version, but simply that we only test it with ghc 9.

Sure but new bugs could be introduced and in that case, depending on their severity, it will be worse than remove support for the plugin

That's true. As maintainers, we always have to balance the cost of duplication versus the perfect coverage. What I am saying is that, as a maintainer, I believe that it is not worth to duplicate these tests and that right now is the optimal moment to upgrade them. But I can also see your point: we don't want to accidentally break GHC 8.x support.

Either now or in the future we are going to need to migrate the test suite to ghc 9. My view is that it's better to do it now that it's cheap rather than wait until later when it's more expensive.

Mmm we will have to

  • copy golden tests and change them and
  • add and cpp (or runtime) if for load files from one location or another

When we will drop support for ghc <= 8, we will have to remove old golden files and the if (like the rest of dozens of similar ifs everywhere).
The extra effort (create the if now and remove later) worths it imo.

CPP in our test suites is not as pervasive as it is in our library code, and as far as I know we don't have duplicate golden tests anywhere else. But I'm not against adding them, if @berberman is willing to contribute them.

@berberman
Copy link
Collaborator Author

I'm fine with maintaining test cases for multi ghc versions, but having many duplicate golden tests files is a bit tedious, so I extracted tests pieces which need variation with ghc versions to the haskell source file for applying conditions.

@berberman berberman marked this pull request as ready for review July 5, 2021 07:32
Copy link
Member

@jneira jneira left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for include the eval plugin for ghc-9

@berberman berberman merged commit 47db34f into haskell:master Jul 5, 2021
@berberman berberman deleted the eval-ghc901 branch July 5, 2021 07:57
@berberman berberman mentioned this pull request Jul 5, 2021
35 tasks
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.

None yet

3 participants