-
Notifications
You must be signed in to change notification settings - Fork 1
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
refactor: Create primer-testlib
and primer-rel8-testlib
packages.
#758
Conversation
1dc9ad3
to
3d3993f
Compare
primer-testlib
and primer-rel8-testlib
packages.
We have numerous testing-related utility functions that are repeated in multiple packages' test suites. This "testlib" package will help us keep them in one place. Note that this commit simply creates the new library and uses it in the `primer` test suite; it is not yet used in the `primer-rel8` or `primer-service` test suites.
This does for `primer-rel8` testing what `primer-testlib` did for `primer` testing.
This wasn't urgent work, but I'm about to start initial work on benchmarks, and I figured it was likely we'd end up with the same sorts of DRY issues for benchmarking as we have for tests, so might as well do this first. |
comprehensive :: App | ||
comprehensive = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this comprehensive
differ from Primer.Examples.comprehensive
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is an App
, the other is a MonadFresh ID m => ModuleName -> m (GVarName, Def)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, I see. This one wraps the Examples
one in an App
-- | ||
-- Note: we should DRY this, see: | ||
-- https://github.com/hackworthltd/primer/issues/273 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused, we haven't actually DRYed anything, just moved code...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, we do that in the next commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were previously 2 copies of this action.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was confused since in the commit where we remove this comment we don't dry, but we do in the very next commit. (And I was looking at the pr commit-by-commit)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, yes, I jumped the gun a bit by removing the comment in the first commit.
Ugh, (edit Sorry for doubting you, |
Fixes #273.