Skip to content

Conversation

iainbeeston
Copy link
Contributor

@iainbeeston iainbeeston commented Nov 8, 2024

Description

Fixes the tags test helper so it returns the text of each visible tag.

The tags helper was getting the label attribute of .tagify__tag elements before but so far as I can see .tagify__tag elements do not have a label. Because of that the tags helper was always returning an array of nil values (one for each tag). Instead I've changed them to get the text of the tag.

I'm not sure why this wasn't causing the avo test suite to fail before, as there is a test for this helper and the other tags helpers (which return the result of the tags helper) but this seems to work for both the avo test suite and my own code (and matches what I can see when I try avo myself in the browser). It also matches what the wait_for_tag_to_appear helper does (ie. looks for tag text, not label).

Fixes # (issue)

The tags test helper was always returning an array of nil values (one for each tag).

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

Screenshots & recording

Manual review steps

So long as the avo test suite still passes this should be ok.

Manual reviewer: please leave a comment with output from the test if that's the case.

It was getting the `label` attribute of `.tagify__tag` elements before but so far as I can see `.tagify__tag` elements do not have a label. Because of that the tags helper was always returning an array of `nil` values (one for each tag).

Instead I've changed them to get the text of the tag, which is what  the `wait_for_tag_to_appear` and  `wait_for_tag_to_disappear` helpers do. This correctly returns the text of the tag.

It looks like this was not spotted before because the only test in the avo test suite that uses the tags helper just checks that an empty array of tags is returned.
Copy link

qlty-cloud-legacy bot commented Nov 8, 2024

Code Climate has analyzed commit c51917a and detected 0 issues on this pull request.

View more on Code Climate.

it "verify tags" do
visit avo.edit_resources_post_path(post)

expect(tags(field: :tags)).to eq ["test", "five", "seven"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like in the test this is coming out as [“seven”, “five”, “test”]

Copy link
Contributor

Choose a reason for hiding this comment

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

True, it was passing locally, but the tags field on the post has enforced suggestions so it's flaky... Just changed it to ["one", "two"] should work as expected not

Copy link
Contributor

@Paul-Bob Paul-Bob left a comment

Choose a reason for hiding this comment

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

I'm not sure why this wasn't causing the avo test suite to fail before, as there is a test for this helper and the other tags helpers (which return the result of the tags helper)

The test case wasn't covering it enough, thanks for this contribution @iainbeeston .

I've committed a test for it.

@Paul-Bob Paul-Bob added the Fix label Nov 11, 2024
@Paul-Bob
Copy link
Contributor

@iainbeeston this helper seems a bit unpredictable, it returns tags in different orders each time. Any ideas why?

@iainbeeston
Copy link
Contributor Author

@Paul-Bob I think that's happening because acts as taggable does not preserve the order of tags by default. For this test to work the way you want you'll need to update Post from using acts_as_taggable_on :tags to acts_as_ordered_taggable_on :tags (documentation is here)

@iainbeeston
Copy link
Contributor Author

The other option would be to use the contains_exactly matcher on the list of tags returned.

@Paul-Bob
Copy link
Contributor

That was it, thanks for looking into this @iainbeeston !

@Paul-Bob Paul-Bob merged commit c9de4df into avo-hq:main Nov 12, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants