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

#394 Add support for viewmatcher #516

Merged
merged 4 commits into from
Dec 18, 2019

Conversation

o-x-y-g-e-n
Copy link
Contributor

Added support for viewmatcher which almost works similar to datamatcher.

args: 'Picture getExternalFilesDir',
class: 'androidx.test.espresso.matcher.ViewMatchers'
})).should.eventually.exist;
await driver.back();
Copy link
Contributor

Choose a reason for hiding this comment

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

please do not make the test dependent on each other. failure of the first test will make all the following to fail

@KazuCocoa
Copy link
Member

cc @dpgraham about viewmatcher

@mykola-mokhnach
Copy link
Contributor

👍 I like the deduplication refactoring

The last questions are these dependent tests. I particularly don't like the fact, that we perform driver.back in several tests in order to setup the view for the following tests. This might break very easily if one of such tests fails. As a possible workaround we could put such calls into finally block.

What do you think @dpgraham ?

@o-x-y-g-e-n
Copy link
Contributor Author

o-x-y-g-e-n commented Dec 6, 2019

👍 I like the deduplication refactoring

The last questions are these dependent tests. I particularly don't like the fact, that we perform driver.back in several tests in order to setup the view for the following tests. This might break very easily if one of such tests fails. As a possible workaround we could put such calls into finally block.

What do you think @dpgraham ?

I am glad you asked it. I actually tried to simulate the scenario where you fail the 1st test case and all the cases immediately after them actually fails. Then, i just followed the way it's done in other test cases.

I would be more than happy to change it and totally isolate them.
There are two solutions for this i think :

  1. Add beforeEach hook and start a new session everytime. (this will increase the runtime)
  2. Put them in try/finally block.

Let me know which one do you guys think is the best way to go?
Thanks.

@mykola-mokhnach
Copy link
Contributor

Add beforeEach hook and start a new session everytime. (this will increase the runtime)

Not all of these tests require additional setup, so I would rather extract these, that work in the same view to a separate sub-suite and reinit the app for all such sub-suites to avoid calling driver.back in any of them in order to setup the env for following tests

@o-x-y-g-e-n
Copy link
Contributor Author

Add beforeEach hook and start a new session everytime. (this will increase the runtime)

Not all of these tests require additional setup, so I would rather extract these, that work in the same view to a separate sub-suite and reinit the app for all such sub-suites to avoid calling driver.back in any of them in order to setup the env for following tests

What i understand from this is let's divide our test cases into two different files.
Supposedly,

  1. find-e2e-specs-default.js -> This will contain all the test-cases for which driver initialization is required only once. (in Before hook).
  2. find-e2e-specs.js -> This will contain all the test cases or suites for which driver initialization is required for all the test cases (in BeforeEach).

Is my understanding correct?

@mykola-mokhnach
Copy link
Contributor

mykola-mokhnach commented Dec 6, 2019

What i understand from this is let's divide our test cases into two different files.

not necessarily. We can still keep the stuff in the same file. The structure might be as following:

describe('independent tests') {
beforeEach() {
startdriver()
}
afterEach() {
quitDriver()
}

...

}

describe('dependent tests') {
before() {
startdriver()
setupView()
}
after() {
quitDriver()
}

...

}

@o-x-y-g-e-n
Copy link
Contributor Author

o-x-y-g-e-n commented Dec 7, 2019

What i understand from this is let's divide our test cases into two different files.

not necessarily. We can still keep the stuff in the same file. The structure might be as following:

describe('independent tests') {
beforeEach() {
startdriver()
}
afterEach() {
quitDriver()
}

...

}

describe('dependent tests') {
before() {
startdriver()
setupView()
}
after() {
quitDriver()
}

...

}

I really dont understand the need of creating a new driver instance every time. I actually tried to write some pseudo code for both logic

  1. Putting driver.back() into finally block
  • The limitation i faced here is that each and every time we do some assertion, that particular statement has to be under try/catch block. Now this will make our code nasty and unpredictable as there can be multiple nested try/catch block.
  1. The approach you mentioned above. But the time for the complete execution increases by a very significant amount.

Conclusion i came upon,
Instead of initializing the driver every time for all the independent test cases, what we can do is we can just reset the app every time (for all the test cases). Now by doing that we have surety that no two test cases are dependent on each other and the app starts from the fresh state. i.e default activity & also I don't need to modify any test cases that are already existing.

`describe.only('find elements', function () {
this.timeout(MOCHA_TIMEOUT);

let driver;
before(async function () {
driver = await initSession(APIDEMO_CAPS);
});
after(async function () {
await deleteSession();
});
// this is the game changer!
this.beforeEach(async function(){
await driver.resetApp();
})
describe('elementByXPath', function () {
it('should find an element by it's xpath', async function () {
let el = await driver.elementByXPath("//*[@text='Animation']");
el.should.exist;
await el.click();
await driver.back();
});
`

@mykola-mokhnach
Copy link
Contributor

resetApp call won't save time, since internally it does exactly the same thing as driver.quit/driver.init.
If it was UIA2 driver then I would use terminateApp/activateApp call, but in case of Espresso the terminateApp call on the app under test would also kill the espresso runner itself.

What we could try is to use deep linking in order to switch between views (e.g. start the corresponding intent for each view). As far as I remember in the Catalog app each entry is represented by a separate activity.

@o-x-y-g-e-n
Copy link
Contributor Author

resetApp call won't save time, since internally it does exactly the same thing as driver.quit/driver.init.
If it was UIA2 driver then I would use terminateApp/activateApp call, but in case of Espresso the terminateApp call on the app under test would also kill the espresso runner itself.

What we could try is to use deep linking in order to switch between views (e.g. start the corresponding intent for each view). As far as I remember in the Catalog app each entry is represented by a separate activity.

I don't know much about deep linking and I think the demo app we are using doesn't support it.
But yes, each entry is represented by an activity. Hence we can use something like,
await driver.startActivity({ appPackage: "io.appium.android.apis", appActivity: ".content.ExternalStorage" })
I will be soon updating all the scenarios with the following code. Does that sound right approach?

@mykola-mokhnach
Copy link
Contributor

Does that sound right approach?

yes it does


before(async function () {
driver = await initSession(APIDEMO_CAPS);
driver.startActivity({
Copy link
Contributor

Choose a reason for hiding this comment

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

this method should be awaited

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot that. So Sorry.

Copy link
Contributor

@mykola-mokhnach mykola-mokhnach left a comment

Choose a reason for hiding this comment

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

Good job. Thanks

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.

None yet

3 participants