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

Adjusted acceptance_helper.rb so that the tests will pass on RBX #402

Closed
wants to merge 2 commits into from

Conversation

thedrow
Copy link
Contributor

@thedrow thedrow commented Jun 5, 2016

No description provided.

@@ -218,7 +218,7 @@ def _add_changes(type, changes, dst)
dst[type].sort!

rescue RuntimeError => e
raise unless e.message == "can't modify frozen Hash"
raise unless e.message == "can't modify frozen Hash" || e.message == "can't modify frozen instance of Hash"

Choose a reason for hiding this comment

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

Line is too long. [111/80]

@coveralls
Copy link

coveralls commented Jun 5, 2016

Coverage Status

Coverage remained the same at 97.691% when pulling 6bb21b8 on thedrow:fix-tests-for-rbx into 2fe70d6 on guard:master.

@e2
Copy link
Contributor

e2 commented Jun 5, 2016

I'd instead use:

raise unless /can't modify frozen(?: instance of)? Hash/ =~ e.message

Also, switch to RBX-3 in .travis.yml. You can remove RBX-2 completely.

@e2
Copy link
Contributor

e2 commented Jun 5, 2016

Just so you know: this likely won't get the tests to pass, but we'll know what the difference in lag time is. If it's too big, it means something is broken. If it's small, it could just be that Travis is doing too much work (e.g. from having too many Listen builds running simultaneously and running out of threads and/or disk access bottlenecks).

Have you tried running the tests locally?

@thedrow
Copy link
Contributor Author

thedrow commented Jun 5, 2016

The tests pass locally with this change. At least now...

@e2
Copy link
Contributor

e2 commented Jun 5, 2016

The tests pass locally with this change. At least now...

Then it was probably just Travis not being fast enough.

The lag value is configurable: https://github.com/guard/listen/blob/master/.travis.yml#L28

Ideally, it's best to know what the lag on Travis is first, before increasing it.

@coveralls
Copy link

coveralls commented Jun 9, 2016

Coverage Status

Coverage increased (+0.1%) to 97.787% when pulling 274de14 on thedrow:fix-tests-for-rbx into 2fe70d6 on guard:master.

@rymai
Copy link
Member

rymai commented Jun 4, 2018

Tests now pass on RBX 3 so I don't think this PR is needed anymore?

@junaruga
Copy link
Contributor

junaruga commented Jun 4, 2018

On current master branch's rbx-3 tests, it seems that some tests are failed. But maybe the coverage logic does not show the result correctly. I think that we should restrict using coveralls for only reliable cases right now.

https://travis-ci.org/guard/listen/jobs/387772775

At least I think RBX has the different message that makes this issue.
See rubinius/rubinius#3663

@junaruga
Copy link
Contributor

junaruga commented Jun 4, 2018

What we can is

  1. Do not use coveralls for rbx test case.
  2. Adapt this PR.

@junaruga
Copy link
Contributor

junaruga commented Jun 5, 2018

My pull-request to RBX to fix the difference of the message was merged to master branch.
rubinius/rubinius@91172ff

@@ -218,7 +218,7 @@ def _add_changes(type, changes, dst)
dst[type].sort!

rescue RuntimeError => e
raise unless e.message == "can't modify frozen Hash"
raise unless /can't modify frozen(?: instance of)? Hash/ =~ e.message
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why we need this in the first place? It seems we .dup each change in #_relative_path: unfrozen_copy = change.dup so that case shouldn't actually happen?

@ioquatix
Copy link
Member

Please rebase and resubmit the PR if it's still relevant.

@ioquatix ioquatix closed this Aug 23, 2020
@thedrow
Copy link
Contributor Author

thedrow commented Aug 24, 2020

Please rebase and resubmit the PR if it's still relevant.

I no longer use Ruby.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants