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

Improve test readability of loadContext and pluginLoaders #1650

Merged
merged 3 commits into from
Feb 8, 2020

Conversation

Beanow
Copy link
Contributor

@Beanow Beanow commented Feb 7, 2020

There was a fair amount of copy-pasted lines in these tests. Which is mostly a
good thing, because ESlint and Flow provide good errors when you're using the
variables wrong. But in terms of readability wasn't great.

In upcoming PRs we'll add more to these test files. So I thought it was good to
improve this first.

This version:

  • Still copy-pastes to get good ESlint and Flow errors.
  • Doesn't repeat itself when it's not for better errors or readability.
  • Uses a "spyBuilder", for readability in spite of prettier trying to collapse lines.
  • Makes sure duplicates are exactly duplicates, easier to edit in IDEs.
    example: [githubDeclaration, "githubDeclaration"]
    instead of [fakeGithubDec, "fake-github-dec"]

Test plan: yarn test

Reviewer note: recommend looking at the split diff on Github, not the unified one.

Copy link
Contributor

@teamdandelion teamdandelion left a comment

Choose a reason for hiding this comment

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

Thanks! I really appreciate cleanup PRs like this. They make me feel good about our codebase. 💜

@Beanow Beanow merged commit b1a6865 into master Feb 8, 2020
@Beanow Beanow deleted the wip/test-clean branch February 8, 2020 07:35
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