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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use `capybara-screenshot` again #3782

Merged
merged 2 commits into from Jul 5, 2018

Conversation

@deivid-rodriguez
Copy link
Contributor

commented Jul 4, 2018

馃帺 What? Why?

A while ago I added a rails monkeypatch so that we can get HTML screenshots on spec failures. The idea was that the patch would be incorporated into Rails and we could eventually get rid of the
monkeypatch. However, the PR has stood there for 5 months now and it doesn't look like this will happen.

capybara-screenshot works just fine, and although it requires a small monkeypatch too, it's much more maintainable than the previous version. So I'd rather go back to it, since the current monkeypatch is too big and specific and might cause trouble in the future.

馃搶 Related Issues

None.

馃搵 Subtasks

None.

馃摲 Screenshots (optional)

None.

A while ago I added a rails monkeypatch so that we can get HTML
screenshots on spec failures. The idea was that the patch would be
incorporated into Rails and we could eventually get rid of the
monkeypatch. However, it doesn't look like this will happen.

capybara-screenshot works just fine, and although it requires a small
monkeypatch too, it's much more maintainable than the previous version.
So I'd rather go back to it, since the current monkeypatch is too big
and specific and might cause trouble in the future.
@ghost ghost assigned deivid-rodriguez Jul 4, 2018
@ghost ghost added the status: WIP label Jul 4, 2018
@deivid-rodriguez

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2018

This PR also happens to implement #2605 (closed as stale, but still a potential improvement), because the feature requested in there is implemented in capybara-screenshot but not in rails.

@deivid-rodriguez

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2018

I'm getting some weird failures but it's strange to me that they're related to this patch. Anybody seen them?

@mrcasals

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2018

Wow, those GraphQL failures are weird. I haven't seen then before, are they consistent? Ie did you try rerunning those jobs, just in case?

@deivid-rodriguez

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2018

Specs run 3 times, one of the current failures happened twice and the other one just this time. The second run hit a different flaky that I fixed. I was confused since it seemed too much flakyness to be unrelated but I don't see how it could be. I guess it was some temporary change in CircleCI that made underlying flakyness apparent. I'll rerun again.

@deivid-rodriguez

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2018

It passed this time!

@mrcasals mrcasals merged commit 2da5a0f into master Jul 5, 2018
28 checks passed
28 checks passed
WIP ready for review
Details
ci/circleci: accountability Your tests passed on CircleCI!
Details
ci/circleci: admin Your tests passed on CircleCI!
Details
ci/circleci: api Your tests passed on CircleCI!
Details
ci/circleci: assemblies Your tests passed on CircleCI!
Details
ci/circleci: blogs Your tests passed on CircleCI!
Details
ci/circleci: budgets Your tests passed on CircleCI!
Details
ci/circleci: build_design_app Your tests passed on CircleCI!
Details
ci/circleci: build_test_app Your tests passed on CircleCI!
Details
ci/circleci: comments Your tests passed on CircleCI!
Details
ci/circleci: consultations Your tests passed on CircleCI!
Details
ci/circleci: core Your tests passed on CircleCI!
Details
ci/circleci: debates Your tests passed on CircleCI!
Details
ci/circleci: generators Your tests passed on CircleCI!
Details
ci/circleci: initiatives Your tests passed on CircleCI!
Details
ci/circleci: main Your tests passed on CircleCI!
Details
ci/circleci: meetings Your tests passed on CircleCI!
Details
ci/circleci: pages Your tests passed on CircleCI!
Details
ci/circleci: participatory_processes Your tests passed on CircleCI!
Details
ci/circleci: proposals Your tests passed on CircleCI!
Details
ci/circleci: sortitions Your tests passed on CircleCI!
Details
ci/circleci: surveys Your tests passed on CircleCI!
Details
ci/circleci: system Your tests passed on CircleCI!
Details
ci/circleci: upload-coverage Your tests passed on CircleCI!
Details
ci/circleci: verifications Your tests passed on CircleCI!
Details
codeclimate All good!
Details
codeclimate/diff-coverage 93% (80% threshold)
Details
codeclimate/total-coverage 98% (0.0% change)
Details
@mrcasals mrcasals deleted the capybara_screenshots branch Jul 5, 2018
@deivid-rodriguez

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2018

It happened again on master: https://circleci.com/gh/decidim/decidim/124930. Maybe this PR is related, eager loading issue due to the monkeypatch?

@mrcasals

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2018

I can't pin-point where we're doing the eager loading, so maybe it's something related to capybara-screenshot itself... Should we revert it then?

@deivid-rodriguez

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2018

I just pushed 22f5c93 that uses the same monkeypatching style we were using before. It's a blind guess, but who knows. I'll see how the build goes, if it fails I'll revert. If it passes, I'll open a PR with the commit. Sounds good?

@mrcasals

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2018

Sounds perfect! Thanks!

deivid-rodriguez added a commit that referenced this pull request Jul 5, 2018
This reverts commit 2da5a0f.

Since it seems to cause a lot of flakies for some reason.
mrcasals added a commit that referenced this pull request Jul 6, 2018
This reverts commit 2da5a0f.

Since it seems to cause a lot of flakies for some reason.
deivid-rodriguez added a commit that referenced this pull request Jul 18, 2018
* Use `capybara-screenshot` again

A while ago I added a rails monkeypatch so that we can get HTML
screenshots on spec failures. The idea was that the patch would be
incorporated into Rails and we could eventually get rid of the
monkeypatch. However, it doesn't look like this will happen.

capybara-screenshot works just fine, and although it requires a small
monkeypatch too, it's much more maintainable than the previous version.
So I'd rather go back to it, since the current monkeypatch is too big
and specific and might cause trouble in the future.
deivid-rodriguez added a commit that referenced this pull request Jul 22, 2018
* Use `capybara-screenshot` again

A while ago I added a rails monkeypatch so that we can get HTML
screenshots on spec failures. The idea was that the patch would be
incorporated into Rails and we could eventually get rid of the
monkeypatch. However, it doesn't look like this will happen.

capybara-screenshot works just fine, and although it requires a small
monkeypatch too, it's much more maintainable than the previous version.
So I'd rather go back to it, since the current monkeypatch is too big
and specific and might cause trouble in the future.
deivid-rodriguez added a commit that referenced this pull request Jul 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can鈥檛 perform that action at this time.