Skip to content

Fixes #982. Upgrade Intern to 3.4.0#1277

Merged
karlcow merged 8 commits intomasterfrom
issues/982/1
Feb 2, 2017
Merged

Fixes #982. Upgrade Intern to 3.4.0#1277
karlcow merged 8 commits intomasterfrom
issues/982/1

Conversation

@miketaylr
Copy link
Copy Markdown
Member

@miketaylr miketaylr commented Jan 11, 2017

  • get it working
  • update docs
  • update travis
  • fix failing tests

Not ready for review yet.

@miketaylr miketaylr force-pushed the issues/982/1 branch 3 times, most recently from 2c1b668 to 76945fe Compare January 11, 2017 16:14
@miketaylr
Copy link
Copy Markdown
Member Author

FYI @karlcow, this is working on my machine™, so you might wait until this is merged to not have to downgrade libs or browsers to run tests locally.

@miketaylr miketaylr force-pushed the issues/982/1 branch 6 times, most recently from d708bb8 to ec1f750 Compare January 11, 2017 21:55
@karlcow
Copy link
Copy Markdown
Member

karlcow commented Jan 12, 2017

❤️ Thanks

@miketaylr
Copy link
Copy Markdown
Member Author

It seems like there's a few tests that are broken now, as a result of changes in either how Intern is working, or Selenium or something. Will try to fix these ASAP and get this merged, this is a major win in terms of simplifying dev environment setup.

@miketaylr miketaylr force-pushed the issues/982/1 branch 2 times, most recently from 51b731a to b8af369 Compare January 13, 2017 23:52
@miketaylr
Copy link
Copy Markdown
Member Author

Locally:

Fx 50 MAC:  [✓✓✓✓✓✓✓✓✓✓✓×✓✓✓×✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓×✓✓✓✓✓] 68/68, 3 fail

Getting there.

@miketaylr
Copy link
Copy Markdown
Member Author

Hmm... 18/68 tests failed. There's some timing differences it seems that I don't understand w/ Intern 3. Things are green locally, but the travis linux VMs either timeout or race.

@miketaylr miketaylr force-pushed the issues/982/1 branch 2 times, most recently from 8f8ce5f to 647432f Compare January 14, 2017 04:46
@miketaylr
Copy link
Copy Markdown
Member Author

43e593d is dumb. But...

5/68 tests failed

There seems to be different behavior when Intern commands "cross" page loads. Like they execute too quickly, or something.

@miketaylr miketaylr force-pushed the issues/982/1 branch 6 times, most recently from d795e75 to 595177e Compare January 15, 2017 00:58
@miketaylr miketaylr force-pushed the issues/982/1 branch 3 times, most recently from 3d6c1ea to 9f748b8 Compare January 27, 2017 05:34
@karlcow karlcow mentioned this pull request Jan 27, 2017
@miketaylr miketaylr force-pushed the issues/982/1 branch 4 times, most recently from 6571844 to 7f022a7 Compare January 27, 2017 19:54
@miketaylr
Copy link
Copy Markdown
Member Author

OK, this is too flaky. @vladikoff said he would help me debug when we're in Mountain View next week.

@miketaylr
Copy link
Copy Markdown
Member Author

OK, this is too flaky. @vladikoff said he would help me debug when we're in Mountain View next week.

We found out there was a bug in my login method that didn't surface w/ Intern 2 -- if we get the extra confirmation page from GitHub, you have to click on a button. In Intern 3, it's clicking on it sometimes too quickly (so possibly we were clicking on the original submit twice, rather than once on submit, then once on confirmation).

Also we have a theory that if you click on a button too quickly, GitHub thinks you're a bot. So, we added a small timeout if you see that extra confirmation page.

Anyways, let's see if the tests actually pass now for both runs.

@miketaylr
Copy link
Copy Markdown
Member Author

All checks have passed

Let's retrigger.

@magsout
Copy link
Copy Markdown
Member

magsout commented Feb 2, 2017

Also we have a theory that if you click on a button too quickly, GitHub thinks you're a bot. So, we added a small timeout if you see that extra confirmation page.

Good catch... hard to find this bug...

@vladikoff
Copy link
Copy Markdown
Member

@magsout hey man, how is it going!? 🥇

Mike Taylor added 6 commits February 1, 2017 21:48
…pers.js.

Before, when Intern was injecting the code to postMessage blobs, Firefox
was throwing an x-ray wrapper permission error, which killed the test.

Presumably this is related to how WebDriver/Intern injects the JS test files.
As a test-only workaround, make it possible to load a test helper file so
methods can be called trusted content.
@miketaylr
Copy link
Copy Markdown
Member Author

r? @karlcow -- not much that's interesting to review in the test re-writes, mostly in the docs and the Python bits.

Copy link
Copy Markdown
Member

@karlcow karlcow left a comment

Choose a reason for hiding this comment

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

Assuming that @vladikoff reviewed the JS with you.
Very very cool to have a helper.js where the functions are defined, thats simplifies a lot the tests code. ❤️

The js folder contains two subfolders: `lib` contains all project source files and `vendor` contains all third party libraries. The files out of the two sub folders contain the compiled source code.

__Note: All code changes should be made to the files in `lib`__
> Note: All code changes should be made to the files in `lib`

This comment was marked as abuse.

Windows typically doesn't have the *make* tool installed. Windows users without *make* should look at the "detailed setup" section below.

As an alternative to Windows, a cloud IDE such as [Cloud 9](https://c9.io) can be used for a relatively easier setup. If you take this route, please update to the latest Python version with the following. (This is to avoid `InsecurePlatformWarning` errors that arise when the default Python 2.7.6 is used).
> Note: If you install Python on Windows using the MSI installer, it is highly recommended to check the "Add to path"-box during installation. If you have not done so, see if one of the answers to the StackOverflow post [Adding Python path on Windows 7](http://stackoverflow.com/questions/6318156/adding-python-path-on-windows-7) can help you - it should also work fine for later versions of Windows.

This comment was marked as abuse.

This comment was marked as abuse.

.then(function(isDisplayed) {
assert.equal(isDisplayed, true, 'Comment form visible for logged in users.');
})
return FunctionalHelpers.openPage(this, url('/issues/100'), '.js-Issue')

This comment was marked as abuse.

'use strict';

var url = intern.config.siteRoot;
var url = intern.config.siteRoot + '/?open=1';

This comment was marked as abuse.

@karlcow
Copy link
Copy Markdown
Member

karlcow commented Feb 2, 2017

And given that github markdown strips the style attribute let's roll it. 🎢

@karlcow karlcow merged commit 2ac4caf into master Feb 2, 2017
@magsout
Copy link
Copy Markdown
Member

magsout commented Feb 2, 2017

@vladikoff hey, fine and you ?

@miketaylr
Copy link
Copy Markdown
Member Author

Thanks @karlcow @vladikoff!

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