ldap (specs): minor improvements using capybara convetions #656
Conversation
Signed-off-by: Vítor Avelino <vavelino@suse.com>
vitoravelino
added
the
no-bugzilla-ref
|
The changes are much cleaner than the method I did previously. I agree that they are an improvement. I still don't understand why the id has to be on the label instead of working on the actual input fields. That does not make sense to me. I would like to know if a full manual test has been done to ensure the page still works from a sanity perspective. |
| @@ -134,67 +134,51 @@ | |||
| describe "#new_save_disabled_by_change" do | |||
| before do | |||
| visit new_settings_dex_connector_ldap_path | |||
| have_button("Save", disabled: true) | |||
| expect(page).to have_button("Save", disabled: true) | |||
There was a problem hiding this comment.
I did test have_button by itself without the expect and it definitely works. Could you explain the difference with that versus adding the expect? Does the expect just repeat the test for the timeout period?
There was a problem hiding this comment.
Could you explain the difference with that versus adding the expect?
The have_button matcher will only look for the element but won't make the test fail if doesn't find what it was looking for. The expect in conjunction with to or not_to will be the one matching your expectations and be responsible to throw an error if it's the case.
So, in this case, the test passed because we were not explicitly expecting have_button to be a truthy matcher. What will decide if it's a truthy or falsy matcher is the to and not_to from the expect.
| # choose id: "dex_connector_ldap_start_tls_true" | ||
| page.execute_script("$('#dex_connector_ldap_start_tls_true').prop('checked', true)") | ||
| page.execute_script("$('#dex_connector_ldap_start_tls_true').trigger('change')") | ||
| find("#dex_connector_ldap_start_tls_true_label").click |
There was a problem hiding this comment.
I do think this is less stable than using jquery. I understand it is the "rspec way" but I feel like it can break ( such as by removing the id from the labels )
There was a problem hiding this comment.
I agree that it feels fragile but what we did here is mimic what the user actually does on the page. The user goes to the page and click on the label "Yes" or "No" that is associated with the input radio.
I still don't understand why the id has to be on the label instead of working on the actual input fields. That does not make sense to me.
As explained above, it's just a matter of reproducing what the user actually does on the page.
The test could also be done using the choose or click directly on the input radio but it wouldn't be 100% true to what is done by the user in the end, which is what we are trying to achieve.
I would like to know if a full manual test has been done to ensure the page still works from a sanity perspective.
Yes, it was done.
| fill_in id: "dex_connector_ldap_bind_pw", with: "AAA" | ||
| page.execute_script("$('#dex_connector_ldap_bind_pw[required=\"required\"]')"\ | ||
| ".trigger('change')") | ||
| fill_in "dex_connector_ldap_bind_pw", with: "AAA" |
There was a problem hiding this comment.
I feel that it warrants a comment in the code here noting that the lookup by id does not work. It is not clear why that should fail.
There was a problem hiding this comment.
It's actually looking by id, I just omitted the id key. The fill_in action will only fail if the dex_connector_ldap_bind_pw element is not found. When using fill_in, the field can be found via its name, id or label text. I think we could have used only "Password" here and it would also work but I didn't realize that in time.
|
It certainly does look cleaner and more correct in regards to Ruby. Pointing to the label does feel more like a 'trick' to me than it should, but better than mixed languages. |
Signed-off-by: Vítor Avelino vavelino@suse.com