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

PIR: restructure tests #5570

Merged
merged 28 commits into from
Oct 3, 2023
Merged

PIR: restructure tests #5570

merged 28 commits into from
Oct 3, 2023

Conversation

michaelpj
Copy link
Contributor

This restructures the tests for PIR following the discussion we had before. Just the test-naming and tasty-discover bits so far. I'll do UPLC and TPLC later.

@michaelpj michaelpj added the No Changelog Required Add this to skip the Changelog Check label Oct 2, 2023
rename
, test_scopingSpoilRenamer genProgram markNonFreshProgram
, T.test_scopingSpoilRenamer genProgram markNonFreshProgram
renameProgramM
-- Can't test substitution procedures at the moment, because that requires generating
-- functions.
, -- The pass breals global uniqueness by design.
Copy link
Contributor

Choose a reason for hiding this comment

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

"breals" typo?

pure . recSplit
, test_scopingGood "renaming" genProgram BindingRemovalNotOk PrerenameNo $
, T.test_scopingGood "renaming" genProgram T.BindingRemovalNotOk T.PrerenameNo $
Copy link
Contributor

Choose a reason for hiding this comment

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

redundant $

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's there for consistency, I think I'm fine with it.

Inline.inline mempty def
, test_scopingGood "match-against-known-constructor" genTerm BindingRemovalNotOk PrerenameNo $
, T.test_scopingGood "match-against-known-constructor" genTerm T.BindingRemovalNotOk T.PrerenameNo $
Copy link
Contributor

Choose a reason for hiding this comment

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

redundant $.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ditto.

Copy link
Contributor

@thealmarty thealmarty left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks!

Just some linter suggestions e.g. redundant bracket for inQuotedName.

Are you still considering the other approach of putting the tests right in src?

@michaelpj
Copy link
Contributor Author

Are you still considering the other approach of putting the tests right in src?

Apparently it doesn't work as well as I'd thought it might. Also, if we did want to do it, it would be quite easy to do on top of this: just merge the source folders! So this gets us 90% of the way there regardless.

@michaelpj
Copy link
Contributor Author

Just some linter suggestions

I'm not going to fix all the lint suggestions in the tests, that's a whole thing in itself 😅

@michaelpj michaelpj merged commit 2a7a949 into master Oct 3, 2023
8 checks passed
@michaelpj michaelpj deleted the mpj/test-restructuring-pir-1 branch October 3, 2023 09:33
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants