thoughtbot / carnival

An unobtrusive, developer-friendly way to add comments
MIT License
501 stars 30 forks source link

Integration testing with Test.Hspec.WebDriver #250

Closed pbrisbin closed 8 years ago

pbrisbin commented 9 years ago

Integration testing via Test.Hspec.WebDriver

This is no fun to run locally. But it's nice to have on CircleCI.

pbrisbin commented 9 years ago

This is ready to be looked at. /cc @jferris

jferris commented 9 years ago

This generally looks good to me, but it's too bad how much boilerplate is required for a single integration test. I suppose some of this could be submitted or extracted to libraries.

pbrisbin commented 8 years ago

@jferris I'm considering closing this as stale. The high ratio of boilerplate to value bothers me. I think I'd rather see the effort go into setting up js unit tests than trying to do something end-to-end that's janky and (probably) very brittle. WDYT?

jferris commented 8 years ago

I think it would be valuable to have some kind of smoke test to ensure we didn't totally break the embedded API. There might be a more lightweight way to do that, though.

pbrisbin commented 8 years ago

OK, I made a few changes and now feel much better about merging this:

Ready for re-review.

jferris commented 8 years ago

That's definitely better. I think my remaining concern is with complicating the cabal setup. I worry about having to add and manage dependencies in two places. I think there's also an issue where having multiple executables breaks yesod devel (see #279).

pbrisbin commented 8 years ago

I think I can get around the duplication and second executable by using a flag to change main-is for the test target. I'll try that out tonight or tomorrow.

On Wed, Nov 11, 2015, 16:36 Joe Ferris notifications@github.com wrote:

That's definitely better. I think my remaining concern is with complicating the cabal setup. I worry about having to add and manage dependencies in two places. I think there's also an issue where having multiple executables breaks yesod devel (see #279 https://github.com/thoughtbot/carnival/issues/279).

— Reply to this email directly or view it on GitHub https://github.com/thoughtbot/carnival/pull/250#issuecomment-155917488.

pbrisbin commented 8 years ago

Well, I can't use a flag to conditionalize main-is because apparently flags can only be used in library or executable stanzas (not test-suite). But I was able to use CPP to make Spec.hs dynamic.

I also got CI green by increasing the wait timeout to 2000. I'm thinking the docs are wrong and it's not actually a value in seconds, maybe milliseconds.

jferris commented 8 years ago

Nice. This is looking really solid. The changes to the non-Selenium stuff are really minimal at this point.

If this works out for us, it looks like there's some generic stuff that could be extract to a library.

pbrisbin commented 8 years ago

This is looking really solid

Solid enough to merge? The only downside I see is it does make CI builds quite a bit slower (~15m -> 30/40m) because we need to recompile a lot as the flags change (which I think is unavoidable).

jferris commented 8 years ago

15m -> 30/40m

Yikes! That's too bad. I think it's worth doing anyway. Maybe using something like Docker can help us out of this mess.

pbrisbin commented 8 years ago

Rebased and updated commit message -- will merge on Green.