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

[#9480] Add snapshot test for student list component #9582

Merged

Conversation

rrtheonlyone
Copy link
Contributor

@rrtheonlyone rrtheonlyone commented Mar 19, 2019

Part of #9480

Added some test cases for the student list component.

No data
Populate with data
Check if data has sections
Check with some fields hidden

@rrtheonlyone rrtheonlyone changed the title [#9480] Add snapshot test for student list page [#9480] Add snapshot test for student list component Mar 19, 2019
Copy link
Contributor

@AyushChatto AyushChatto left a comment

Choose a reason for hiding this comment

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

Add cases for :-

  • enableRemindButton set to true
  • student with photoUrl
  • isAllowedToModifyStudent set to true

@rrtheonlyone rrtheonlyone added the s.Ongoing The PR is being worked on by the author(s) label Mar 21, 2019
@rrtheonlyone rrtheonlyone force-pushed the instructor-list-snapshot branch 2 times, most recently from b227b5c to 7390a4f Compare March 26, 2019 11:31
@rrtheonlyone
Copy link
Contributor Author

Added the respective cases @AyushChatto
(except for photoUrl - that one will be removed after #9575)

Copy link
Contributor

@AyushChatto AyushChatto left a comment

Choose a reason for hiding this comment

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

LGTM as far the coverage is concerned. Had some qns about the snapshot though, but I might be wrong. Best to err on the side of caution I suppose.

</tm-student-list>
`;

exports[`StudentListComponent should snap with enable remind button set to true and one student yet to join 1`] = `
Copy link
Contributor

Choose a reason for hiding this comment

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

Really minor nit, but there's actually two students that are yet to join.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup updated!

</tm-student-list>
`;

exports[`StudentListComponent should snap with some student list data and some students to hide 1`] = `
Copy link
Contributor

Choose a reason for hiding this comment

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

Next time, make life easier for reviewers by giving the hidden students names like 'This should not be displayed' :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I am not writing this test to make life easier for reviewers. I have fed in data that you are more likely to see in production for this application. I have preferably tried to make it as real as possible (so we are more likely to catch bugs).

As a reviewer, you could have looked out for:

    component.listOfStudentsToHide = [
      'alice.b.tmms@gmail.tmt',
      'tester@tester.com',
    ];

From there you would have to make sure that the student with this email, is not going to be present in this snapshot.

It is definitely a little bit more work and I agree that names like 'This should not be displayed' would have made things a little faster (its also not super critical to have names like Alice). I can change this if you prefer but personally, I would stick to data that is as close to production if I could.

If you follow a pattern like this, you may end up compromising the quality of your tests at some point..but again this is just my own opinion! I am okay either way :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Just basing it off #9605 - I haven't traditionally follow it either, but I kinda see the merit in it. No worries, this is fine, just for future reference anyway :)

@rrtheonlyone
Copy link
Contributor Author

I have improved as per the suggestions. @AyushChatto have a look when you have time.

Copy link
Contributor

@AyushChatto AyushChatto left a comment

Choose a reason for hiding this comment

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

LGTM :)

@AyushChatto AyushChatto added s.FinalReview The PR is ready for final review and removed s.Ongoing The PR is being worked on by the author(s) labels Mar 29, 2019
Copy link
Member

@wkurniawan07 wkurniawan07 left a comment

Choose a reason for hiding this comment

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

LGTM

@wkurniawan07 wkurniawan07 added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.FinalReview The PR is ready for final review labels Apr 3, 2019
@rrtheonlyone rrtheonlyone merged commit 7e3fa93 into TEAMMATES:master Apr 7, 2019
@rrtheonlyone rrtheonlyone deleted the instructor-list-snapshot branch April 7, 2019 15:58
@wkurniawan07 wkurniawan07 added this to the V7.0.0-beta.0 milestone Apr 10, 2019
@wkurniawan07 wkurniawan07 added this to Ongoing in Front-end and RESTful back-end Migration via automation Apr 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants