Edit vocabulary lessons in dialog #38839
Conversation
|
thanks for the gif! I see what looks like an autocomplete search box, but it only shows you scrolling down without typing. am I correct in understanding that typing into this box allows you to filter what appears in the dropdown? (apologies, I have not pulled this down to try it out yet!) |
|
@davidsbailey yes you can type -- though the search is a bit different than the one on the lesson edit page. I'm not sure how it works exactly -- it comes for free with the component. This gif is a lot longer and shows a lesson actually updating, as well as typing into the select box. |
| expect(wrapper.contains('Add Vocabulary')).to.be.true; | ||
| expect(wrapper.find('input').length).to.equal(2); |
There was a problem hiding this comment.
I think @Hamms has convinced me that these kinds of "brittle expectations" are not very helpful for the following reasons:
- they can break when someone adds a new input
- due to the shallow render, they can break when someone wraps an existing component (button, input, etc) within another subcomponent
- they do not test business logic
- they are mostly just testing that the React library is working
Generally, they make tests brittle without adding much value. For now, can I recommend removing lines 111 and 112?
Personally, I think there is some value in still having the "renders default props" test case, because in the event of a later test failure, whether this test case fails can immediately help narrow down the root cause. However, this is only valuable if other test cases go on to test actual business logic (which I am not commenting on one way or another in this comment). So let's leave that in for now, but we may keep it on the table whether to remove "renders default props" test cases entirely in the future.
There was a problem hiding this comment.
"Add Vocabulary" does actually test business logic because the header is different in different cases. I see the point in removing line 112 so I'll go ahead and do this
There was a problem hiding this comment.
Ah, great, thank you for pointing that out! 👍
There was a problem hiding this comment.
"Add Vocabulary" does actually test business logic because the header is different in different cases
It does! But I'd argue that means it should belong in a test that's specifically testing that the header is different in different cases, not be disguised as a test that claims to just be testing our ability to render
|
I'm not sure if this is new, but after pulling down your branch I am running into this issue where the vocab update succeeds, but then the lesson update fails: edit-vocab.mp4 |
| @@ -31,6 +31,20 @@ class VocabulariesControllerTest < ActionController::TestCase | |||
| assert(@response.body.include?('updated definition')) | |||
| end | |||
|
|
|||
| test "can update lessons from params" do | |||
There was a problem hiding this comment.
nit: "can update vocab from params"?
| assert_equal 'updated definition', vocabulary.definition | ||
| assert(@response.body.include?('updated definition')) | ||
| assert_equal [lesson], vocabulary.lessons | ||
| end |
There was a problem hiding this comment.
This test looks good! I think it would be a good idea to add a test for saving the lesson after saving the vocab (issue shown in video in other comment). I'm not sure how easy it would be to add a rails unit or integration test, but it should be possible to cover it with a UI test, based on the UI tests we already have for saving the lesson edit page. Up to you whether that belongs in this PR.
There was a problem hiding this comment.
I'd like to look into that bug separately and add tests there.
Follow up from a comment in #38760 -- this took less than a day :). This PR adds a selector to edit which lessons a vocabulary is in from the All Vocabulary page. It doesn't affect the lesson edit page.
Quick gif -- I'm not sure why the save was slow but I believe that was my computer being sluggish
Future work
Testing story
Manual testing and a couple unit tests
Reviewer Checklist: