Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix all broken tests #484

Merged
merged 6 commits into from
Feb 6, 2020
Merged

Fix all broken tests #484

merged 6 commits into from
Feb 6, 2020

Conversation

Kornil
Copy link
Contributor

@Kornil Kornil commented Jan 21, 2020

Fixes #23

PR Checklist

  • My branch is up-to-date with the Upstream master branch.
  • The unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Short description of what this resolves:

  • In the first commit of this PR, I updated the failing snapshots on the assumption that code changes are correct (since they were already on master) and the snapshots not updated.
  • The second commit fixes the 4 broken tests in stories/Tooltip/Tooltip.stories.js. Reactstrap UncontrolledTooltip tries to queries the body, since we don't have one being in a test environment, I simulated having it with the correct ids.
    This fixes the test while not changing the stories. Storybook still behaves as it should, using the rendered elements to display tooltips.
    see Tooltip cannot find ID of a React element rendered next to it reactstrap/reactstrap#773 (comment) for the issue.
  • The third commit is just snapshots being updated
  • The fourth commit same fix for stories/Avatar/AvatarLinkWithTooltip.js
  • The fifth commit same fix but for stories/Templates/servizi/Servizi.js

Open to feedback.

  • Nel primo commit ho aggiornato gli snapshot assumendo che il codice è corretto (dato che è in master) mentre gli snapshot no.
  • Il secondo commit fixa 4 test in stories/Tooltip/Tooltip.stories.js. Reactstrap UncontrolledTooltip cerca gli id su body e non trovandoli manda in errore prima di poter vedere gli elementi corretti. Come fix ho simulato gli id su div invisibili su document, questo fixa i test senza cambiare niente su storybook. Tutti i tooltip continuano a funzionare sugli elementi specificati inizialmente.
    Tooltip cannot find ID of a React element rendered next to it reactstrap/reactstrap#773 (comment) per il problema.
  • Il terzo commit semplice update di snapshot
  • Il quarto stesso fix su stories/Avatar/AvatarLinkWithTooltip.js
  • Il quinto stesso fix ma su stories/Templates/servizi/Servizi.js

Fatemi sapere.

- reactstrap UncontrolledTooltip searches the document object for ids at first render, by adding invisible divs with the required ids th tests will pass and on render the tooltips will be correctly applied to the tests' correct buttons reactstrap/reactstrap#773 (comment)
- Update snaps
@Kornil Kornil changed the title Fix/tests Fix - tooltip tests Jan 21, 2020
@Kornil Kornil changed the title Fix - tooltip tests Fix all broken tests Jan 21, 2020
@dej611
Copy link
Member

dej611 commented Jan 21, 2020

Thanks @Kornil for the submission! 🎉

I will have a look at it and comment here in case of any change request!

stories/Templates/servizi/Servizi.js Show resolved Hide resolved
stories/Tooltip/Tooltip.stories.js Show resolved Hide resolved
stories/Avatar/AvatarLinkWithTooltip.js Show resolved Hide resolved
@Kornil
Copy link
Contributor Author

Kornil commented Jan 27, 2020

Thank you for the feedback @dej611, and sorry for the late reply, I'm having a busy week.
While I agree with your comments, there is an issue between testing reactstrap elements and storyshots not using the dom as storybook does.

I might be able to use this to make it look a bit better but it will look very similar.

Let me know your opinion.

@dej611
Copy link
Member

dej611 commented Jan 29, 2020

I see.
Then either using the alternative approach, or leave some comments on those lines to explain the issue and point to the issue at storybook. Better with a TODO thing to refactor later.

@Kornil
Copy link
Contributor Author

Kornil commented Feb 2, 2020

I could not find a way to use createNodeMock to fix these tests so I opted for a clear TODO comment explaining the issue and the nature of the fix.

To reduce codebase pollution, I moved all the fixes to the respective .stories files. This way the components themselves stay clean and do not interact with the application in unexpected ways. It restores the purity the components had before so we can deploy safely.

@dej611 let me know what you think. While exploring I found a few more issues to address around testing but I will open new PRs so this one stays scoped to only fix the broken tests.

@Kornil Kornil requested a review from dej611 February 2, 2020 17:54
@dej611
Copy link
Member

dej611 commented Feb 4, 2020

I checked out your last commits, while they are much better in a code prospective, they pose an issue with the Storybook tool.

For instance, take this Storybook component:

const AvatarLinkWithTooltipFix = () => {
  // TODO find a better way to handle this
  // Storyshot does not use the dom so can't render refs
  // to fix the problem we append the elements manually
  // this fixes tests without touching the rendered components
  // nor storybook
  // https://github.com/storybookjs/storybook/issues/886
  // https://github.com/infinitered/addon-storyshots#using-createnodemock-to-mock-refs
  ids.map((id, i) => {
    const div = document.createElement('div')
    div.setAttribute('id', id)
    document.body.appendChild(div)
  })

  return <AvatarLinkWithTooltip />
}

While this looks much better as the original source code is untouched, in the Storybook info panel this is the actual source code that will be shown :(

May I propose to rollback the last 3 commits and add the comments on the stories themselves? I know its a step back from before but at least we can give users some directions about the specific context and to ignore those lines for their development.

@Kornil
Copy link
Contributor Author

Kornil commented Feb 4, 2020

Thank you for the feedback, I reverted the last 3 commits and added the comments where I placed the fix originally.

Copy link
Member

@dej611 dej611 left a comment

Choose a reason for hiding this comment

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

Good job @Kornil 👍

@dej611 dej611 merged commit 60601de into italia:master Feb 6, 2020
@Kornil Kornil deleted the fix/tests branch February 6, 2020 17:43
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.

Define a testing strategy
2 participants