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

[#11572] Add notification banner E2E test (E2E in individual pages) #11749

Merged
merged 4 commits into from
Apr 17, 2022
Merged

[#11572] Add notification banner E2E test (E2E in individual pages) #11749

merged 4 commits into from
Apr 17, 2022

Conversation

NicolasCwy
Copy link
Contributor

@NicolasCwy NicolasCwy commented Apr 17, 2022 •
edited by wkurniawan07
Loading

Fixes #11572

Outline of Solution
Unlike #11748, this PR tests the presence of the notification banner in the notification page as well as the home page for both instructors and students. Notification banner functionality is tested in NotificationBannerE2ETest.

@NicolasCwy NicolasCwy mentioned this pull request Apr 17, 2022
Closed
@NicolasCwy NicolasCwy force-pushed the 11572-notification-feature-notification-banner-refactored-e2e branch from 3e81650 to 7f89c49 Compare April 17, 2022 08:52
@jianhandev jianhandev added this to the V8.14.0 milestone Apr 17, 2022
@jianhandev jianhandev added s.Ongoing The PR is being worked on by the author(s) c.Task Other non-user-facing works, e.g. refactoring, adding tests labels Apr 17, 2022
@NicolasCwy NicolasCwy force-pushed the 11572-notification-feature-notification-banner-refactored-e2e branch 2 times, most recently from 88cf39a to 33f4ab5 Compare April 17, 2022 09:11
jianhandev
jianhandev approved these changes Apr 17, 2022
View reviewed changes
Copy link
Contributor

@jianhandev jianhandev left a comment

Choose a reason for hiding this comment

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

LGTM, except for a small change request:

src/e2e/java/teammates/e2e/pageobjects/AppPage.java Outdated Show resolved Hide resolved
@jianhandev jianhandev added s.ToReview The PR is waiting for review(s) and removed s.Ongoing The PR is being worked on by the author(s) labels Apr 17, 2022
@NicolasCwy NicolasCwy force-pushed the 11572-notification-feature-notification-banner-refactored-e2e branch from 33f4ab5 to 4259a9c Compare April 17, 2022 10:46
wkurniawan07
wkurniawan07 approved these changes Apr 17, 2022
View reviewed changes
Copy link
Member

@wkurniawan07 wkurniawan07 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I can't say I like the way the dedicated E2E test is designed, but I don't have a better alternative as of now.

src/e2e/java/teammates/e2e/pageobjects/AppPage.java Outdated Show resolved Hide resolved
@NicolasCwy NicolasCwy added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.ToReview The PR is waiting for review(s) labels Apr 17, 2022
@NicolasCwy NicolasCwy merged commit 87ae891 into TEAMMATES:master Apr 17, 2022
@NicolasCwy NicolasCwy deleted the 11572-notification-feature-notification-banner-refactored-e2e branch April 17, 2022 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jianhandev jianhandev jianhandev approved these changes

@wkurniawan07 wkurniawan07 wkurniawan07 approved these changes

Assignees

@wkurniawan07 wkurniawan07

@jianhandev jianhandev

Labels
c.Task Other non-user-facing works, e.g. refactoring, adding tests s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Milestone
V8.14.0
Development

Successfully merging this pull request may close these issues.

Notification Feature
3 participants
@NicolasCwy @wkurniawan07 @jianhandev