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

Version-sensitive plutus-benchmark tests (PLT 7544) #5587

Merged
merged 14 commits into from
Oct 18, 2023
Merged

Conversation

kwxm
Copy link
Contributor

@kwxm kwxm commented Oct 12, 2023

This is PLT-7544: "Run plutus-benchmark tests on both 9.2 and 9.6". The golden tests in plutus-benchmark now all have two lots of results, one for GHC 9.2 and one for GHC 9.6.

@kwxm kwxm added Test No Changelog Required Add this to skip the Changelog Check labels Oct 12, 2023
@@ -12,6 +12,10 @@
{-# OPTIONS_GHC -fno-omit-interface-pragmas #-}
{-# OPTIONS_GHC -fno-spec-constr #-}
{-# OPTIONS_GHC -fno-specialise #-}
{-# OPTIONS_GHC -fexpose-all-unfoldings #-}
Copy link
Contributor Author

@kwxm kwxm Oct 12, 2023

Choose a reason for hiding this comment

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

We should remember to get someone to look at this. Like it says in the comment underneath, there's a ticket at PLT-7976.


-- | Run a 'TestTree' of tests with a given name prefix.
runTestNestedIn :: [String] -> TestNested -> TestTree
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''m not sure if I like this name. It sounds like it runs a test, but really it's running a computation in the Reader monad.

testNestedGhc folderName = testNested (folderName </> ghcVersion)

-- Create a TestTree which runs in the directory 'path/<ghc-version>/dir'
runTestGroupNestedGhcIn :: [FilePath] -> FilePath -> [TestNested] -> TestTree
Copy link
Contributor Author

@kwxm kwxm Oct 12, 2023

Choose a reason for hiding this comment

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

I'm conflicted about whether to represent paths like ["dir1", "dir2", "dir3"] or "dir1/dir2/dir3". We use both of these in the codebase (and even in this file), but in theory we might be in trouble with the latter on a system that doesn't use "/" as the path separator. That's presumably very unlikely though. Anyway, I didn't want to embark on a massive irrelevant-to-this-PR refactoring to make everything consistent.

Copy link
Member

Choose a reason for hiding this comment

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

The first options seems strictly better. I'm not sure whether or not the second option works on Windows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first options seems strictly better.

Maybe we can do that in a later PR. You can work around it though, like this.

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 found some tests in plutus-ir that had paths like ["a/b/c"] so I changed them to ["a", "b", "c"] while I was at it.

testNestedGhc folderName = testNested (folderName </> ghcVersion)

-- Create a TestTree which runs in the directory 'path/<ghc-version>/dir'
runTestGroupNestedGhcIn :: [FilePath] -> FilePath -> [TestNested] -> TestTree
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 don't like this name very much, but I couldn't think of anything better.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why this is needed. There's doesn't seem to be a use case where dir isn't []?

Copy link
Contributor Author

@kwxm kwxm Oct 16, 2023

Choose a reason for hiding this comment

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

Oh, that's wrong. It says it puts it in path/version/dir, but in fact it puts it in path/dir/version.

This gets used in marlowe/test and lists/test. See here for example, where the directory is semantics. You end up with golden files in plutus-benchmark/marlowe/test in the following directories:

  • role-payout/9.2
  • role-payout/9.6
  • semantics/9.2
  • semantics/9.6

What I meant to do in that function was to move the version directories further up, so you'd have

  • 9.2/role-payout
  • 9.2/semantics
  • 9.6/role-payout
  • 9.6/semantics

I don't know if that's any better. Maybe we should just leave the directory layout as it is and get rid of the final dir argument in that function.

Copy link
Contributor Author

@kwxm kwxm Oct 18, 2023

Choose a reason for hiding this comment

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

I've fixed this now: all of the 9.2/9.6/etc directories should be at the leaves of the directory hierarchy (so we're using the first option above).


-- Make a set of golden tests with results stored in subdirectories determined
-- by the GHC version.
testGroupGhc :: [TestNested] -> TestTree
Copy link
Contributor Author

@kwxm kwxm Oct 12, 2023

Choose a reason for hiding this comment

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

... or maybe versionedTestGroup or something. I feel that it should mention versions somewhere.

Copy link
Member

@zliu41 zliu41 left a comment

Choose a reason for hiding this comment

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

Looks good. When merging, please use a better commit message than "Kwxm/test/plt 7544".

testNestedGhc folderName = testNested (folderName </> ghcVersion)

-- Create a TestTree which runs in the directory 'path/<ghc-version>/dir'
runTestGroupNestedGhcIn :: [FilePath] -> FilePath -> [TestNested] -> TestTree
Copy link
Member

Choose a reason for hiding this comment

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

The first options seems strictly better. I'm not sure whether or not the second option works on Windows.

testNestedGhc folderName = testNested (folderName </> ghcVersion)

-- Create a TestTree which runs in the directory 'path/<ghc-version>/dir'
runTestGroupNestedGhcIn :: [FilePath] -> FilePath -> [TestNested] -> TestTree
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why this is needed. There's doesn't seem to be a use case where dir isn't []?

@kwxm kwxm changed the title Kwxm/test/plt 7544 Version-sensitive plutus-benchmark tests (PLT 7544) Oct 18, 2023
@kwxm kwxm merged commit 056383e into master Oct 18, 2023
6 checks passed
@kwxm kwxm deleted the kwxm/test/PLT-7544 branch October 18, 2023 23:56
zliu41 added a commit that referenced this pull request Nov 22, 2023
This release contains backport of the change to PlutusLedgerApi.V1.Value (adding `-fexpose-all-unfoldings` from #5587.
zliu41 added a commit that referenced this pull request Nov 22, 2023
This release contains backport of the change to PlutusLedgerApi.V1.Value (adding `-fexpose-all-unfoldings`) from #5587.
@zliu41 zliu41 mentioned this pull request Nov 22, 2023
zliu41 added a commit that referenced this pull request Nov 22, 2023
This release contains backport of the change to PlutusLedgerApi.V1.Value (adding `-fexpose-all-unfoldings`) from #5587.
zliu41 added a commit that referenced this pull request Nov 22, 2023
This release contains backport of the change to PlutusLedgerApi.V1.Value (adding `-fexpose-all-unfoldings`) from #5587.
locallycompact pushed a commit that referenced this pull request Nov 24, 2023
This release contains backport of the change to PlutusLedgerApi.V1.Value (adding `-fexpose-all-unfoldings`) from #5587.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
No Changelog Required Add this to skip the Changelog Check Test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants