Skip to content

Conversation

this-self
Copy link
Contributor

No description provided.

@this-self this-self requested review from nightnei and StyleT May 25, 2021 15:32
import chai from 'chai';
import sinon from 'sinon';

describe('WrapApp', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. You're missing test cases for unmount (when we have wrapper or app rendered).
  2. I would try to write utility function to check state of all callbacks at once. Ex. expectBallbacksToBe(callbacks, 'mounted')
  3. Separately we need to test the fact that App Wrapper always receives getCurrentBasePath() = '/', unique appId and it's own props while wrapped app receives "own" props from registry.
  4. We also need to check that class also works with callbacks provided as array of functions. See #canonizeCallbacks

@this-self
Copy link
Contributor Author

@StyleT I have made changes regarding your comments.
Much detalised each case and allocated tests with "arrayed" callbacks as a different tests.

Probably, would be good to group them either:

  • by general scenarios: boostrap/mount/unmount wrapper, bootstrap/mount/unmount wrapped app, dynamic render wrapped app after wrapper has been rendered
  • or by callbacks approaches, e.g.: normal and "arrayed".

For now leaved as is.

Other moment is about expectations check of arrayed callbacks arguments. I have hardcoded them manually to be able to track the error on exact line. If you prefer to have there an additional abstraction layer - I will do, let me know.

@this-self this-self merged commit e48834c into master May 27, 2021
@this-self this-self deleted the addTestsForClientWrappApp branch May 27, 2021 17:11
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.

3 participants