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.

Sign up for GitHub

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

Jump to bottom

Async testing #21

Merged
merged 6 commits into from
Dec 7, 2020
Merged

Async testing #21

merged 6 commits into from
Dec 7, 2020

Conversation

CalebCyphers
Copy link
Collaborator

@CalebCyphers CalebCyphers commented Dec 5, 2020 •
edited
Loading

What does this PR do?

  • builds Tests for all dynamic components

Where should the reviewer start?

  • App.test.js
  • SelectedMovie.test.js

How should this be manually tested?

  • npm test

What are the relevant tickets?

@CalebCyphers CalebCyphers added enhancement New feature or request testing U KNO WHAT TESTING IS U DINGUS refactor labels Dec 5, 2020
@CalebCyphers CalebCyphers requested a review from demaceo December 5, 2020 21:17
demaceo
demaceo reviewed Dec 7, 2020
View reviewed changes
src/App.js
@@ -55,6 +36,11 @@ class App extends Component {
/>
</Col>
</Row>
<Row className="d-flex justify-content-center">
<Col md="auto">
{this.state.error && <Alert variant="danger">{this.state.error}</Alert>}
Copy link
Owner

Choose a reason for hiding this comment

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

conditional rendering ftw!

demaceo
demaceo reviewed Dec 7, 2020
View reviewed changes
src/Components/NavBar/NavBar.test.js

const returnHome = jest.fn();

describe("NavBar", () => {
Copy link
Owner

Choose a reason for hiding this comment

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

future note: For Iteration 5, we could add a search input into the navbar

demaceo
demaceo approved these changes Dec 7, 2020
View reviewed changes
Copy link
Owner

@demaceo demaceo left a comment

Choose a reason for hiding this comment

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

Let's add some SCSS for the NavBar and Selection Movie.
Aside form that though, it looks/acts marvelous... absolutely marvelous!

@demaceo demaceo requested a review from letakeane December 7, 2020 16:26
@demaceo demaceo merged commit 80ba8cb into main Dec 7, 2020
letakeane
letakeane reviewed Dec 10, 2020
View reviewed changes
Copy link
Collaborator

@letakeane letakeane left a comment

Choose a reason for hiding this comment

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

Looks like you also need an integration test that checks that when a movie is clicked, the details are rendered! My guess is that App will need to do this test.

src/App.js
@@ -13,31 +13,12 @@ class App extends Component {
super();
this.state = {
movies: [],
selectedMovie: 0,
selectedMovie: null,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can the initial value for this be an empty object instead of null? Always best to avoid mutating the data type.

src/App.js
@@ -91,6 +77,25 @@ class App extends Component {
</React.Fragment>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Fun fact: you can create a react fragment with the shorthand <> </>

Copy link
Owner

Choose a reason for hiding this comment

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

that is a fun fact!

src/App.js
componentDidMount() {
getMovies()
.then((response) => this.setState({ movies: response.movies }))
.then(this.setState({ loader: false }))
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the loading message is dependent solely on if there are movies rendered or not, we can remove this part of state and just check on this.state.moves! However if more than one condition can set this to true, then it can stay.

Copy link
Owner

Choose a reason for hiding this comment

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

duly noted!

src/App.js
getMovies()
.then((response) => this.setState({ movies: response.movies }))
.then(this.setState({ loader: false }))
.catch((err) => this.setState({ error: err }));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Be sure that "err" is a string as the initial state indicates, and not an object with an error message inside it.

src/App.js

returnToHome = (event) => {
this.setState({
selectedMovie: 0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like the default value of selectedMovie should be 0 instead of null!

src/Components/MovieCard/MovieCard.test.js

// should be able to click on a movie card
Copy link
Collaborator

Choose a reason for hiding this comment

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

I LOVE the pseudocoding! This is a great workflow!

src/Components/MovieCard/MovieCard.test.js

userEvent.click(movieCardPoster);

expect(handleClick).toHaveBeenCalled();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great unit test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@demaceo demaceo demaceo approved these changes

@letakeane letakeane letakeane left review comments

Assignees

@CalebCyphers CalebCyphers

@demaceo demaceo

Labels
enhancement New feature or request refactor testing U KNO WHAT TESTING IS U DINGUS
Projects
None yet
Milestone
Iteration 2 - Displaying an individua...
3 participants
@CalebCyphers @letakeane @demaceo