Adds a spec on BOM pagination #10383
Conversation
filipefurtad0
force-pushed
the
add_pagination_BOM_spec
branch
2 times, most recently
from
0586497
to
673e0c6
Compare
|
I'm missing this section in my tests: Switching it into a draft. |
filipefurtad0
force-pushed
the
add_pagination_BOM_spec
branch
from
673e0c6
to
98a6403
Compare
|
@filipefurtad0 |
filipefurtad0
force-pushed
the
add_pagination_BOM_spec
branch
from
5f6ef07
to
93f4aa8
Compare
|
This will need a rebase after merging #10391 and getting the build green again. Keeping it In Dev for now. |
filipefurtad0
force-pushed
the
add_pagination_BOM_spec
branch
from
93f4aa8
to
23c60bf
Compare
@jibees do you still observe this issue? I've tested quickly on Safari 13 and could not observe it, so maybe something with the test environment. After rebasing build is green (except for the flaky Jest specs), moved to code review |
|
Build is still red. Did you actually rebased? I wanted to rebase you're PR but seems like I don't have the authorization, strange, this happens for the first time. and spec pass. This was on my machine as expected ;) |
filipefurtad0
force-pushed
the
add_pagination_BOM_spec
branch
3 times, most recently
from
53cf6c9
to
bfa1605
Compare
No idea why that happened on this branch...
Great, so the issue is resolved 👍 |
| it "splits results according to line items from orders" do | ||
| visit_bulk_order_management | ||
|
|
||
| expect(page).to have_select2 "autogen4", selected: "15 per page" # should be default option |
There was a problem hiding this comment.
Where does "autogen4" come from? Could that change when we edit other parts on the page? Is there a better identifier like a label?
Update: I just checked and our matcher requires an id while our code doesn't provide one. So we have to do with this auto-generated id for now. Fortunately, we are replacing AngularJS and select2 and this doesn't need fixing.
There was a problem hiding this comment.
Thanks - yes, I believe that's the case.
we are replacing AngularJS and select2 and this doesn't need fixing
Ok, so I guess when we do, these tests will need to be updated?
this doesn't need fixing
Agree, let's leave as it is. Sometimes I feel page elements need more consistent and more explicit labeling/identifiers for this type of testing; is also influences Matomo tracking (element clicking, for ex.). I might ping you or @jibees on this matter, soon again.
There was a problem hiding this comment.
Ok, so I guess when we do, these tests will need to be updated?
Yes, that's the downside of select2 because it needs special Rspec matchers. If it would work with the normal select matchers then we wouldn't need to change it. The new library, tom-select does work with the default matchers. So if we replace it at some point in the future, the specs hopefully don't need changing.
Sometimes I feel page elements need more consistent and more explicit labeling/identifiers for this type of testing; is also influences Matomo tracking (element clicking, for ex.).
I agree that we could have more labels. In this case, we could also use the text next to it as label which says about how many are display per page. Then you would be able to click on that to open the select input.
| expect(page).to have_button("« First", disabled: true) | ||
| expect(page).to have_button("Previous", disabled: true) | ||
| expect(page).to have_button("1", disabled: true) | ||
| expect(page).to_not have_button("2", disabled: false) |
There was a problem hiding this comment.
Negative matchers should be the least specific possible. Otherwise you could have a page with this button disabled which would still be wrong. You want to assert that there is no "2" button, no matter if it's disabled or not.
| expect(page).to_not have_button("2", disabled: false) | |
| expect(page).to_not have_button("2") |
There was a problem hiding this comment.
Thanks, great catch for that detail 👍
Committed and squashed.
Adds coverage on bottom section of pagination Extracts order line item creation to before block Removes disabled option when asserting on button 2
filipefurtad0
force-pushed
the
add_pagination_BOM_spec
branch
from
8df6842
to
c84ec14
Compare
What? Why?
Adds coverage on pagination, on the BOM page.
What should we test?
Release notes
Changelog Category: Technical changes
The title of the pull request will be included in the release notes.
Dependencies
Documentation updates