GPII-1889: Initial review of gpii-webdriver... #1
Conversation
…tion, and script execution.
…g test in Firefox.
… to a non-conflicting name and exposed the original functions.
…s passed to `executeAsyncScript`.
| # | ||
| # if you want to change the VMs specifications go to README.md | ||
|
|
||
| VAGRANT_VMENV_PATH = `node -e "require('vagrant-vmenv')"`.delete!("\n") |
There was a problem hiding this comment.
Seems problematic to me. We should just install the module locally as a result of npm install - as well as avoiding the failure due to faulty use of "require" this will also remove the requirement for the funny message. This approach may well of course lead to problems if the "node_modules" directory ends up being aliased inside and outside the VM. One solution to this may be to perform the activity inside a "monorepo" style nested project which simply holds the vagrant-vmenv dependency itself. Either way, this dependency needs to be installed locally, not globally.
There was a problem hiding this comment.
I agree with the concerns, and have created a ticket against the QI project, where this needs to be addressed:
|
Just to comment on the use of the "failing tests" pattern. In this package, I provide a means of retrieving jqUnit results from a browser. I want to test that both failures and successes in the browser are correctly retrieved. Those particular browser tests are meant to visibly fail and be logged within the browser, so it seems like the pass/fail inversion would not be appropriate here. |
|
I have merged the selenium 3.x work with this, which also brings in the ESLint standard configuration. This should ensure that the tests work at least with "vagrant up" on your machine. I am spinning up a Windows VM separately and will confirm that the tests run in a more comparable environment. |
|
I used the Vagrant windows image to confirm that the Chrome tests run in Windows. |
src/tests/helper-functions.js
Outdated
|
|
||
|
|
||
| gpii.test.webdriver.axs.runAxs = function (axsOptions) { | ||
| /* globals axs */ |
There was a problem hiding this comment.
I'd prefer to see this moved up to the head of the file - given a substantial purpose of these definitions (as well as suppressing the lint warning) is to announce to the reader of a file that it contains global references
| var fluid = require("infusion"); | ||
| var gpii = fluid.registerNamespace("gpii"); | ||
|
|
||
| require("../../../"); |
There was a problem hiding this comment.
These test requires could more intelligibly be replaced by fluid.require("%gpii-webdriver") as well as then becoming resistant to the dir structure being reorganised
There was a problem hiding this comment.
Done, throughout the tests, including requires of package-relative materials such as test fixtures.
| args: ["{that}", "wait", "onWaitComplete", "{arguments}"] | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Missing EOL here, as well as a couple of other linting failures in the pull (comma-dangle)
There was a problem hiding this comment.
It's so bizarre, I shouldn't even be able to commit with lint errors. I must have managed to break my ESLint config in just the right way at just the right time for a previous commit to succeed.
tests/js/wait/index.js
Outdated
|
|
||
| gpii.tests.webdriver.wait.confirmTimeoutMessage = function (message, expectedText) { | ||
| // I have to require this here or the whole suite will time out with "no tests found" errors. | ||
| // TODO: Review and discuss with Antranig. |
There was a problem hiding this comment.
There are two main timeout hazards in the system - firstly the 13ms timeout which is in QUnit https://github.com/fluid-project/infusion/blob/master/tests/lib/qunit/js/qunit.js#L484 - responsible for draining its queue and deciding whether it is empty, and then secondly the 100ms timeout which is in node-jqUnit, determining whether it will attempt to restart - https://github.com/fluid-project/node-jqunit/blob/master/lib/jqUnit-node.js#L209 - we should try to see which of these you are running foul of.
In another context (a similar problem with the IoC testing framework as a whole) I patched an "interlock flag" into our edition of QUnit which will prevent it from exiting in the case some external driver has determined that "tests should be arriving" - https://github.com/fluid-project/infusion/blob/master/tests/lib/qunit/js/qunit.js#L1465 - it's possible that using this flag might be helpful to you
There was a problem hiding this comment.
I removed the workaround and required jqUnit from the top of the file. The problem is no longer there, so I assume that a) it was the 13ms timeout we were hitting and b) the first for FLUID-5810 is enough for us.
tests/js/get/remote.js
Outdated
| }] | ||
| }); | ||
|
|
||
| // gradeNames: ["fluid.test.testEnvironment"], |
tests/js/fluid/html/index.html
Outdated
| }; | ||
|
|
||
| var namespaced = { | ||
| fn: function(){ return true; } |
There was a problem hiding this comment.
If this file could have been linted, the spacing would fail here - I'd say that even these definitions are worth putting in their own file
There was a problem hiding this comment.
Done. In moving this to another file, I ended up namespacing the previously global functions, which made the final check (which tested the use of deeper paths) redundant.
|
Current state of the pull looks very good and I was able to run the tests without problems. Looks like just a last few bits of snagging |
|
@amb26, the latest feedback has been addressed. |
…ost-2.0.0 binder transform functions and deletion relay fix.
…pendencies based on output of `yarn outdated`.
See GPII-1889 for details.