Skip to content

Consolidated Javalab Editor refactors#47486

Merged
sanchitmalhotra126 merged 13 commits intostagingfrom
sanchit/javalab-editor-refactor/consolidated
Aug 8, 2022
Merged

Consolidated Javalab Editor refactors#47486
sanchitmalhotra126 merged 13 commits intostagingfrom
sanchit/javalab-editor-refactor/consolidated

Conversation

@sanchitmalhotra126
Copy link
Copy Markdown
Contributor

@sanchitmalhotra126 sanchitmalhotra126 commented Aug 3, 2022

This PR contains the remaining Javalab editor refactor changes that were originally branched off of Ben's refactors which have now been merged (PR here). The below PRs/changes are included in this PR, and have each been reviewed independently. The only additional changes here are some minor merge conflict fixes and test updates.
Included changes:

There have been a lot of PRs, branching, and merging with these changes, so I'm also including a full work log for clarity and future reference :) hopefully this should make tracking down changes easier in the future.

Full Work Log

  1. Ben opened JavalabEditor refactor: move editor state into Redux #46970. This was reviewed but kept un-merged since we expected to add more refactors before updating tests.
  2. I opened the following PRs (also listed above), the first branched off of Ben's change above, and each subsequently branched off of each other. Each of these was also reviewed but kept un-merged for the same reason above.
    Move dialog state into redux #47053
    Move compile status state management into CommitDialog #47082
    Move Javalab Editor header and dialogs into their own components #47116
    Move file error message into redux #47137
  3. Each of the four PRs above, once reviewed, was merged into the PR it was branched off of. The final PR (Move dialog state into redux #47053) was merged back into Ben's original PR (JavalabEditor refactor: move editor state into Redux #46970).
  4. In retrospect, we realized that Ben's original change, and my subsequent changes didn't have much logical overlap, so we decided to update tests and merge them separately.
  5. Ben opened and merged Isolated JavalabEditor Redux refactor #47445 which contained only the original changes from JavalabEditor refactor: move editor state into Redux #46970, plus test fixes.
  6. I opened this PR Consolidated Javalab Editor refactors #47486 which contains the remaining changes.

Links

Testing story

Tested locally.

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@sanchitmalhotra126 sanchitmalhotra126 requested a review from a team August 4, 2022 00:58
@sanchitmalhotra126 sanchitmalhotra126 changed the title Sanchit/javalab editor refactor/consolidated Consolidated Javalab Editor refactors Aug 4, 2022
@sanchitmalhotra126 sanchitmalhotra126 marked this pull request as ready for review August 4, 2022 01:07
Copy link
Copy Markdown
Contributor

@bencodeorg bencodeorg left a comment

Choose a reason for hiding this comment

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

Thank you for the PR description, very helpful :)

.undefined;
expect(store.getState().javalab.sources[oldFilename]).to.be.undefined;
expect(javalabEditor.state.openDialog).to.be.null;
expect(store.getState().javalab.editorOpenDialogName).to.be.null;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Opinion I came up with when looking at these tests earlier this week: we should check the prop of the component that references this value (rather than Redux itself). Thoughts?

Copy link
Copy Markdown
Contributor Author

@sanchitmalhotra126 sanchitmalhotra126 Aug 4, 2022

Choose a reason for hiding this comment

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

Yeah, I definitely agree - I was trying to do that initially but the issue I was running into is that we don't actually read the state value (editorOpenDialogName) in the component. We only call the associated action (openEditorDialog/closeEditorDialog). I was trying to instead just assert that we called those functions, but I couldn't figure out how to do that...passing the functions as custom props didn't seem to work because we use redux inside the test so I think those functions get overwritten. Any ideas?

One option could be to remove redux entirely for this file (or at least for the tests where we're running into this issue) and just assert that it calls the right action handlers. But we use redux for example to check that certain sources were updated so we'd have to change all that too.

Edit: reading a bit more about Redux in tests and I think in the ideal case it might be best to have two separate tests; one that's more a true unit test that only tests the functions of JavalabEditor without using Redux, and an integration test that tests the editor features end to end, and uses Redux. Right now this test is doing a bit of both?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Gotcha, I see now. Re: Redux in tests, seems like most (all?) tests here require Redux. I feel like for a non-reusable component like this that uses Redux in our actual app, relying on Redux in tests is reasonable since that is how we actually use it? For a more reusable component, isolating tests from Redux would make more sense to me?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think that's fair. I think as pull more stuff out of this component into smaller sub-components, those sub-components can have more isolated unit tests (even if they're technically not reusable) and then this test could serve more as a full integration test?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That makes sense to me, so for something like JavalabEditorDialogManager (if we were to add tests) we'd try to use no Redux? Do you have any rules of thumb about deciding when you might want to have a component fully isolated from Redux vs not?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think that sounds right? I don't have a rule of thumb necessarily, but I think it makes sense that we test smaller, functional components without redux because that introduces a lot of overhead and leaks into areas (i.e. the Redux store) that the component doesn't need to be concerned about - as opposed a component like this (JavalabEditor) that's more complicated and needs to ensure that the UI is being updated correctly.

.undefined;
expect(store.getState().javalab.sources[oldFilename]).to.be.undefined;
expect(javalabEditor.state.openDialog).to.be.null;
expect(store.getState().javalab.editorOpenDialogName).to.be.null;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Gotcha, I see now. Re: Redux in tests, seems like most (all?) tests here require Redux. I feel like for a non-reusable component like this that uses Redux in our actual app, relying on Redux in tests is reasonable since that is how we actually use it? For a more reusable component, isolating tests from Redux would make more sense to me?

@sanchitmalhotra126 sanchitmalhotra126 merged commit fb406f3 into staging Aug 8, 2022
@sanchitmalhotra126 sanchitmalhotra126 deleted the sanchit/javalab-editor-refactor/consolidated branch August 8, 2022 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants