-
Notifications
You must be signed in to change notification settings - Fork 68
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
Clean up new testsuite #114
Conversation
…e Common.App in place of Config.App
…pre/post 4.7 base
Snap.Snaplet.Common.EmbeddedSnaplet | ||
Snap.Snaplet.Common.FooSnaplet | ||
Snap.Snaplet.Common.Handlers | ||
Snap.Snaplet.Common.Types |
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.
Can we rename these modules to make it clear that they belong to the testsuite? (I.e. Snap.Snaplet.Test.Common.*)
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.
For sure. I can probably get to this in about an hour.
Thanks for doing all this cleanup work! |
Sorry for taking so long to review+merge this, I've been moving between countries. |
@@ -2,6 +2,8 @@ | |||
|
|||
set -e | |||
|
|||
cp -r test/snaplets ./ |
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 don't love this, can you fix it to not litter files in the root directory?
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's the complementary clean-up line on line 71, deleting that directory after testing
https://github.com/imalsogreg/snap/blob/master/runTestsAndCoverage.sh#L71
I tried a bit to get the test snaplet to look in a directory other than project's root for the snaplet directory, but didn't manage to make it work. Keep trying?
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.
Can you do a "cd test" so that everything moves into the test directory?
@gregorycollins No worries about the slow merge. There are a lot of changes to check (and I see that this now has merge conflicts for some reason). Thanks for patience w/ some of my oversights (like -fhpc flag in the library). That's now fixed. Feedback's been great, feel free to keep letting me know if you need more changes before you pull it in. |
Yeah, this package is currently in a bit of flux because of my changes to heist. Can you try merging master into your branch? If any conflicts come up that you don't understand, let me know and I'll help with them. |
cabal sdist
and rebuilding from the snap-1.0.0.0.tar.gz to make sure all files are getting packaged up