-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[#58426] add feature spec for custom fields of type hierarchy #16988
base: dev
Are you sure you want to change the base?
[#58426] add feature spec for custom fields of type hierarchy #16988
Conversation
fdeea52
to
58d570e
Compare
- https://community.openproject.org/work_packages/58426 - add a couple of test selectors - add page objects
58d570e
to
851273c
Compare
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.
Bunch of questions, rants and of course, no suggestions. 🤣
#++ | ||
|
||
require "spec_helper" | ||
require "support/pages/custom_fields/hierarchy_page" |
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 support folder is already required by spec_helper
, this shouldn't be needed.
app/components/admin/custom_fields/hierarchy/delete_item_dialog_component.html.erb
Show resolved
Hide resolved
render Primer::Beta::Blankslate.new(border: true) do |component| | ||
render Primer::Beta::Blankslate.new( | ||
border: true, | ||
test_selector: "op-custom-fields--hierarchy-items-blankslate" |
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.
🟡 Any reason this isn't a constant like the other one?
@@ -31,6 +31,7 @@ See COPYRIGHT and LICENSE files for more details. | |||
primer_form_with( | |||
url: custom_field_items_path(@custom_field), | |||
method: :post, | |||
data: { test_selector: "op-custom-fields--new-item-form" } |
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.
🟡 Same question as above
render Primer::Alpha::Banner.new(scheme: :default, | ||
icon: :info, | ||
dismiss_scheme: :hide, | ||
test_selector: "op-custom-fields--new-hierarchy-banner") do |
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.
🟡 Same same.
|
||
alias_method :custom_field, :model | ||
|
||
def has_no_items_or_projects? |
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 leverage the render?
method
<%= f.text_field :name, | ||
required: true, | ||
container_class: "-middle", | ||
"data-test-selector": "op-custom-fields--new-custom-field-name" %> |
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.
🟡 More non-constant selectors.
let(:new_custom_field_page) { Pages::CustomFields::NewPage.new } | ||
let(:hierarchy_page) { Pages::CustomFields::HierarchyPage.new } | ||
|
||
it "lets you create, update and delete a custom field of type hierarchy", |
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.
🔴 Wouldn't be better to split those cases into separate 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.
No, IMHO that's exactly the opposite of what we should do with e2e tests.
Reasoning:
The ramp up of the test suite takes ages, compared to the actual test run. This is why I wrote the work with the custom field like a story. You start by creating it, do a lot of nasty stuff to it, and then you end by deleting it. Also, an expensive test like this is meant to test UI and integration, not all of the use cases. Which is why I kept some things out for now.
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'd separate them to avoid eventual state leaks to change the behaviour.
I tend to see feature specs as user stories with clear intent in each test and acceptance criteria for it. We may need to involve others in this discussion.
@apfohl @brunopagno WDYT?
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.
In the past I was bitten by having a single/few tests because not all errors would be visible right away. Then you get to the situation where you fix something and it explodes again further into the test.
I understand the argument against, though, that tests might take much longer.
I guess it's a question if we can "pay the price" of longer running 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.
I see also two sides. The one side don't wanting an even longer running test suite and the other side wanting to have easier maintainable specs. It seems like you can't have both at the moment. Honestly I would say we should go the way of doing it like @Kharonus. Define the domain story (you could even draw it) and play that thing through. Maybe we could use # region REGION_NAME
# endregion
to visually highlight the steps. The IDE allows to collapse them.
In general I would not delay on feature spec development to much. Invest more into automated domain model unit testing.
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 separating the test into clearly distinct regions
something you would approve @mereghost ?
end | ||
end | ||
|
||
def expect_empty_items_banner(visible:) |
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 have mixed feeling about hidden assertions in helpers as makes the tests kinda opaque. That being said, it is just me so don't bother too much with 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.
My goal was to make the test itself easily readable without bothering to much about capybara syntax. Things like click_on
and fills_in
are already doing that quite nicely, but the stuff about test_selectors add nothing to the readability imho.
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.
click_on
and fill_in
represent user actions.
This sets an expectation/assertion making it completely invisible to the test as you scan for expect(thing).to eq(zeug)
making parsing what is being tested pretty hard. =/
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 it then a general opposition of using POs? They are meant to encapsulate these kind of things into more readable things.
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.
Nah, I'm cool with PO's... even with managers. :P
Seriously, POs are ok the issue is the hidden assertion. When parsing specs, I search for expect(something).to something
and not page.expect_something
.
I classify this as worse than shared specs used only in one place.
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.
okay, understood. I'll recheck the code with having this kind of glasses on.
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 we can extend the expect
language with new custom matchers. Not sure if that is easily doable. I'm thinking of some kind of fluent syntax extension:
expect(page).when(:visible).to have(BANNER).when_not(:visible).not_to have(BANNER)
Or different.
Ticket
OP#58426
also fixes: OP#58466
What are you trying to accomplish?
What approach did you choose and why?