Simplify status caching #366
Conversation
|
This looks good to me. The version of |
|
@imurchie where it would be better to put this test (into which file/suite)? Are there some useful examples of similar tests? |
|
The functional test should be in here: https://github.com/appium/appium-xcuitest-driver/blob/master/test/functional/basic/basic-e2e-specs.js#L24 The unit test in here: https://github.com/appium/appium-xcuitest-driver/blob/master/test/unit/driver-specs.js#L21 |
test/unit/driver-specs.js
Outdated
| beforeEach(() => { | ||
| d = new XCUITestDriver(); | ||
| sandbox = sinon.sandbox.create(); | ||
| jwproxyCommandSpy = sandbox.stub(d.wda.jwproxy, "command", async () => { |
There was a problem hiding this comment.
Does this run? At this point d.wda is unintialized, I think. You probably need something like
d.wda = new WebDriverAgent({major: 8, minor: 2}, {});
d.wda.setupProxy(42);
|
|
||
| let result; | ||
| B.Promise.any( | ||
| [new B(driver.elementsByClassName)('WrongLocator'), new B(driver.status)()] |
There was a problem hiding this comment.
No need for creating new promises. Without await both driver.elementsByClassName() and driver.status() return promises already.Also, does B.any(no need for the Promise) guarantee that the first element in the array is called first? I can't find anything indicating that. If it does not, then sometimes this would not test what it is supposed to test.
There was a problem hiding this comment.
changed. Yep, this algorithm won't work as expected
mykola-mokhnach
force-pushed
the
simplify_status_caching
branch
3 times, most recently
from
840392a to
d25f7f3
Compare
lib/utils.js
Outdated
| @@ -117,6 +120,27 @@ async function checkAppPresent (app) { | |||
| log.debug('App is present'); | |||
| } | |||
|
|
|||
| async function getGitRev () { | |||
There was a problem hiding this comment.
This function will not do what you want it to, I don't think. It will, if it returns anything, return the containing project (i.e., run through the full Appium project, it will give the get revision for the Appium project, not the driver).
There was a problem hiding this comment.
Ok, I'll exclude this data then
| proxySpy.calledOnce.should.be.true; | ||
| proxySpy.firstCall.args[0].should.eql('/status'); | ||
| proxySpy.firstCall.args[1].should.eql('GET'); | ||
| describe('status caching', () => { |
There was a problem hiding this comment.
This test fails (https://travis-ci.org/appium/appium-xcuitest-driver/jobs/204494918#L2438). Do you not run the pre-commit hooks before committing? This should have been caught there.
There was a problem hiding this comment.
It takes time to run this stuff on local machine. I usually test it once and then rely on Travis output while fixing it. Although this one took quite a lot of time for Travis to finish the execution :(
There was a problem hiding this comment.
Yeah, Travis has been overloaded of late. But the unit tests and eslint ought to be quick, and catch much.
mykola-mokhnach
force-pushed
the
simplify_status_caching
branch
from
912b38b to
14311d2
Compare
This PR allows to get rid of unnecessary flag in XCUITestDriver class and make /status response caching more reliable. The PR depends on appium/appium-base-driver#105