-
Notifications
You must be signed in to change notification settings - Fork 530
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
feat(test): POC e2e tests #3998
Conversation
); | ||
|
||
// Select "Apple" brand in list | ||
const brand = await browser.$('span=Apple'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason this isn't querySelector
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not in the browser context here so we have to use the selector from WebdriverIO. It is a bit different than querySelector
https://webdriver.io/docs/selectors.html
// Wait for the results list to be updated (wait for the "macbook" word to be highlighted) | ||
await browser.waitUntil( | ||
async () => (await browser.$$('mark=MacBook')).length > 0, | ||
10000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we not rely on the search callback instead of a magic number?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by search callback? You can find the API documentation here https://webdriver.io/docs/api.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can wait for the result
event of search
for example. However that isn't very portable between frameworks. Since we know there's only one search done, the search client could be injected & waited for maybe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it is an E2E test we're not supposed to interact with InstantSearch differently from a user.
We should only wait for a visual feedback that the search is done, like a real user.
); | ||
|
||
// Select "Apple" brand in list | ||
const brand = await browser.$('span=Apple'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rely on something else than span
? This seems flaky and we shouldn't describe where to get the information, but rather what information to get. In that case we want the label
(this is what drives accessibility) element having as value "Apple"
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The label
element have the value Apple\n442
and the linebreak seems to trigger some issues with the selector (I did not take the time to try to fix it).
But we can also use CSS selectors if needed. https://webdriver.io/docs/selectors.html
Targeting the right elements is tricky in E2E test as the UI and/or CSS classes can change frequently.
IMHO in our case we should rely on CSS classes, since they are pretty stable in InstantSearch (any change would be a breaking change).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish to be able to use something like ByLabelText here :(
If span
sounds flaky, IMO, the class names are less flaky since we try to maintain all the same class names across the flavors(which is far from a good way to test in accessibility-wise, and which is testing implementation details).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we use them for E2E tests we can imagine the usage for [test-id]
attribute in the DOM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's also a possibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test- attributes could be nice by I don't see how we can do it without altering every the DOM of every widget.
We're pretty lucky our css classes are pretty stable across flavours, using them should be good enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this can help: https://github.com/testing-library/testcafe-testing-library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly this is not compatible with WebdriverIO 😞
|
||
// Get title for each result | ||
const hits = await browser.$$('.hit h1'); | ||
const hitsText = await Promise.all(hits.map(hit => hit.getText())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we extract only the text and not the HTML for also the highlighted parts? This seems to cover better the actual usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I'm only testing that I got the right list of results, I do not test that the text is correctly highlighted.
hostname: 'hub.browserstack.com', | ||
port: 443, | ||
baseUrl: '', | ||
user: process.env.BROWSERSTACK_USER, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure yet if these E2E tests are also meant to be run on our local machines. If so, we can consider adding dotenv
to our stack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the e2e tests fail on CI server, we'll probably want to try it again on our local machines. So let's consider adding it :)
'browserstack.networkLogs': true, | ||
}, | ||
|
||
capabilities: [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the time impact of adding multiple other combinations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends of the offer on the test service.
With an offer like Automate Pro on BrowserStack we can run 5 tests in parallel, so 5 combinations in total without impacting the test duration.
browser.maximizeWindow(); | ||
}); | ||
|
||
it('must work', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For end to end test, I'd tend to use it
in a slightly different way that in a unit test, for readability of the report and to clarify the intent of the commands that failed.
Example in this case:
it('navigates to the e-commerce demo', () => {
await browser.url('http://localhost:5000/examples/e-commerce/');
});
it('selects Apple in the brands refinement list', () => {
const brand = await browser.$('span=Apple');
await brand.click();
});
I'm just suggesting. This test is small enough that it's doesn't beg for it 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not have any opinion on this, it makes the test a bit longer but one advantage of this is that it annotates the test, for example on Sauce Labs you'll get:
(this is similar to the use of sauce:context
)
|
||
// Compare them to expected titles | ||
expect(hitsText).to.deep.equal([ | ||
'Apple - MacBook Air® (Latest Model) - 13.3" Display - Intel Core i5 - 8GB Memory - 128GB Flash Storage - Silver', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I think what we expect is to have Apple in all the hits text so maybe we should just check that?
There are less changes the test will fail if for some reason the ranking a or hit changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmm, I prefer to test that we get exactly the results we expect, and checking for only "Apple" is too vague IMO.
I find this as a good thing to have the test to fail if the ranking or a hit change 😇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something we can do with a dedicated demo: we can show the value of the attribute that will be filtered in the UI so we can test it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be equivalent to exposing some internals for testing, not a good practice and it would ruin the purpose of having end-2-end tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really true, the attribute is what proves that it's filtered, since the filter isn't related to the search query per se, and can be in multiple attributes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not forget that we are supposed to test from a end user POV, so what matter at the end is that the result list is correct.
Summary
This PR contains a sample End-2-End test and a configuration for each tested service.
See related gist for details and instructions about how to run tests on each service.