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

Refactor integration test to not require standards violating repeated element ID #212

Merged
merged 3 commits into from
Mar 14, 2013

Conversation

mjtko
Copy link
Contributor

@mjtko mjtko commented Mar 13, 2013

The chatbox template can't really contain an ID for the textarea, as it is rendered once per channel. This PR removes the need to give it an ID just for the sake of the test.

Additionally I found I had to refactor the port on which Faye is set up to listen during tests as my system already has 9292 in use.

@@ -36,6 +36,7 @@
config.active_support.deprecation = :stderr

# Variable set to be able to get faye client for test environments
ENV['FULL_HOST'] = "http://localhost:9292"
ENV['KANDAN_FAYE_PORT'] = "9292" unless ENV['KANDAN_FAYE_PORT']
ENV['KANDAN_FAYE_URL'] = "http://localhost:#{ENV['KANDAN_FAYE_PORT']}" unless ENV['KANDAN_FAYE_URL']
Copy link
Member

Choose a reason for hiding this comment

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

Why KANDAN_FAYE_URL vs KANDAN_HOST? I don't think that variable name is very descriptive of the actual value of the variable since it is not faye specific

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... My understanding was that it is Faye specific though! Where is it used for anything other than Faye?

Copy link
Member

Choose a reason for hiding this comment

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

It is only used there now but may be used in other places later and since the variable value is not really faye specific it may be confusing in the future. No big deal anyways. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In which case, I'd suggest that we rename it when/if we use it elsewhere. 😄

Copy link
Member

Choose a reason for hiding this comment

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

👍

@coveralls
Copy link

Coverage increased (+0.08%) when pulling 9ea95bc on mjtko:fix/repeated-id into fc93ad8 on kandanapp:master.

View Details

@gabceb
Copy link
Member

gabceb commented Mar 13, 2013

It looks like travis build failed https://travis-ci.org/kandanapp/kandan/builds/5482464

@gabceb
Copy link
Member

gabceb commented Mar 13, 2013

Any idea why this failed? It passes the test on my machine and it passes on #213. I cant figure out how to restart the job on Travis.

@fusion94
Copy link
Member

@mjtko
Copy link
Contributor Author

mjtko commented Mar 14, 2013

I don't think this is Ruby 2.0 specific. There's some kind of race condition in the integration test. This is what my colleagues and I refer to as an 'FFBT' or F***ing Flaky Browser Test.

I'll see if I can make it more reliable, but it passes fine on my machine under Ruby 2.0.0 too.

@gabceb
Copy link
Member

gabceb commented Mar 14, 2013

I will merge this and we can deal with the racing condition on a separate PR. Ok @mjtko ?

@mjtko
Copy link
Contributor Author

mjtko commented Mar 14, 2013

Let's see if that commit solves it. I think it should do, but it's difficult to tell outside of the Travis environment as everything passes on my machine anyway!

@coveralls
Copy link

Coverage increased (+0.08%) when pulling 6f2b972 on mjtko:fix/repeated-id into fc93ad8 on kandanapp:master.

View Details

@mjtko
Copy link
Contributor Author

mjtko commented Mar 14, 2013

Looks like that's done it - or at least it passed that time around. Hopefully that's fixed the FFBT. 😉

gabceb added a commit that referenced this pull request Mar 14, 2013
Refactor integration test to not require standards violating repeated element ID
@gabceb gabceb merged commit 82cfd2c into kandanapp:master Mar 14, 2013
@gabceb
Copy link
Member

gabceb commented Mar 14, 2013

Ohhhh yeah 😄

ar7em pushed a commit to ar7em/kandan that referenced this pull request Jun 9, 2013
Refactor integration test to not require standards violating repeated element ID
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.

4 participants