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

Instant log output for failing UI tests and some tweaks on detection of page activity #8094

Merged
merged 6 commits into from Jun 16, 2015

Conversation

tsteur
Copy link
Member

@tsteur tsteur commented Jun 12, 2015

  • If there is a failure we do now output JS errors, not loaded resources, ... immediately within the tests. Even if the UI tests do not finish within 50 minutes we still get an idea of what is going on. It prints the result at the bottom of the test again but as it is only for failing tests shouldn't matter so much. Code wise we could have copied the mocha spec reporter and adjusted this one but can still do it later if needed. There's no coloured output for this yet so hope that's okay. It should help us to troubleshoot things.
  • I noticed we detect whether there are any ajax requests running and capture a screenshot only if all ajax requests and images are loaded. We did not check for JS resources and neither for AngularJS requests (there are quite a few of them) which can lead to random test results. I added a check for the AngularJS requests. As the detection of loaded images and requests can be buggy I thought it would be nice to use PhantomJS's events onResourceRequested, onResourceReceived, ... This way we would not have to take care of this in core and we can easily print a list of not finished requests at the end of the test. Unfortunately we have some synchronous requests and for those requests no "response" event will be triggered meaning we cannot detected that the sync request is finished. Therefore I do now check whether either phantomjs detected that all resources are loaded or our core logic. Once we refactored Rewrite all synchronous XHR as they are deprecated #8020 we can trust PhanomJS. This will allow us to detect more accurately when a page is loaded or stopped being active etc.
  • While working on this I noticed the Menu tests were wrong. There was a test for Admin Menu but it was actually User Menu tests so I changed it and added a test.

An example can be seen here :

The tests that fail seem to also fail on master?!?

@tsteur tsteur added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Jun 12, 2015
@tsteur tsteur added this to the 2.14.0 milestone Jun 12, 2015
@@ -49,8 +49,6 @@ describe("Login", function () {

it("should redirect to login when logout link clicked", function (done) {
expect.screenshot("login_form").to.be.capture("logout_form", function (page) {
page.click("#topBars span.title:contains(superUserLogin)");
Copy link
Member Author

Choose a reason for hiding this comment

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

Did not work before. This was code from when the username opened a dropdown menu

@tsteur tsteur added Needs Review PRs that need a code review and removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels Jun 12, 2015
@mattab mattab added the c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. label Jun 12, 2015
if (!self.aborted) {
self.pageLogs.push('Unable to load resource (#' + resourceError.id + 'URL:' + resourceError.url + ')');
self.pageLogs.push('Error code: ' + resourceError.errorCode + '. Description: ' + resourceError.errorString);
if (self._isUrlThatWeCareAbout(resourceError.url)) {
Copy link
Member

Choose a reason for hiding this comment

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

I think you can combine this if w/ the outer one.

Copy link
Member Author

Choose a reason for hiding this comment

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

will do

@diosmosis
Copy link
Member

Once we refactored #8020 we can trust PhanomJS. This will allow us to detect more accurately when a page is loaded or stopped being active etc.

Is it possible to detect the synchronous request and display a warning in phantomjs? Or even fail the test? Would perhaps help in finding all the synchronous requests.

@diosmosis
Copy link
Member

Did you try and reduce the default wait time? W/ the new wait detection logic, maybe it could be lowered.

@mnapoli
Copy link
Contributor

mnapoli commented Jun 15, 2015

I haven't reviewed the code diff but the change seems useful (e.g. the example builds you linked to).

I added a check for the AngularJS requests

👍

As the detection of loaded images and requests can be buggy I thought it would be nice to use PhantomJS's events onResourceRequested, onResourceReceived

Are "images not fully loaded" a problem sometimes? Sounds good in theory but I wonder whether it's worth the effort?

@diosmosis
Copy link
Member

Not sure what the travis issue is, but when I try to run the ActionsDataTable UI tests locally I get two timeouts. After I finish looking at other PRs, I'll look into this more closely.

@tsteur
Copy link
Member Author

tsteur commented Jun 15, 2015

Are "images not fully loaded" a problem sometimes? Sounds good in theory but I wonder whether it's worth the effort?

Yes, we will be able to detect much more accurately whether all resources are loaded. Eg we currently do not detect whether *.js, font files are loaded etc. Also the current logic goes throw every DOM node all the time and checks whether there are images defined via 'backgroundImage', 'listStyleImage', 'borderImage', 'borderCornerImage', 'cursor' and checks whether they are loaded or not. It can easily happen that we don't detect all images this way and not accurately. This way random build failures can happen. Also we will be able to run tests faster at some point by detecting better when the page finished being active. Also if we forget to reset some ajax requests in core some pages might not finish loading. Using PhantomJS it will just work (in theory g)

@tsteur
Copy link
Member Author

tsteur commented Jun 15, 2015

Is it possible to detect the synchronous request and display a warning in phantomjs? Or even fail the test? Would perhaps help in finding all the synchronous requests.

I don't think we can detect them, I had a quick look. It will be quite a bit of work to refactor them as we will now have to use callbacks everywhere a lot of code needs to be changed. We can just search for send(true) in the code base and should find them

@tsteur
Copy link
Member Author

tsteur commented Jun 15, 2015

Did you try and reduce the default wait time? W/ the new wait detection logic, maybe it could be lowered.

We should test in a separate branch after all tests are green. It's currently hard to tell since many UI tests fail randomly. It would be awesome if we could lower it

// why isEmpty(self._resourcesRequested) || !self._getAjaxRequestCount()) ?
// if someone sends a sync XHR we only get a resoruceRequested event but not a responseEvent so we need to
// fall back for ajaxRequestCount as a safety net. See https://github.com/ariya/phantomjs/issues/11284
self._executeEvents(events, callback, i + 1);
Copy link
Member

Choose a reason for hiding this comment

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

There's an issue here, since it's now a nested if, if the second condition is false, the function just exists. So _waitForNextEvent isn't called again in that case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thx! will update

diosmosis added a commit that referenced this pull request Jun 16, 2015
Instant log output for failing UI tests and improved detection of page activity. Also includes fixed and new menu tests.
@diosmosis diosmosis merged commit 86a74fa into master Jun 16, 2015
@diosmosis diosmosis deleted the ui_test_framework_tweaks branch June 16, 2015 02:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants