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

[#12048] Migrate student notification page e2e test #12773

Merged

Conversation

cedricongjh
Copy link
Contributor

@cedricongjh cedricongjh commented Feb 22, 2024

Part of #12048

Outline of Solution
Persist ReadNotifications in data bundle
Add removeAndStoreSqlDataBundle to e2e base test case
Split out the non-course related entities into its own bundle to be loaded
Migrated the page object methods to use sqlentity
Removed some code that checked if account was migrated for certain actions
Return SqlDataBundle so we can read the ID

@cedricongjh cedricongjh added this to the V9.0.0-beta.0 milestone Feb 22, 2024
Comment on lines +354 to +360
.orShould().accessClassesThat(new DescribedPredicate<>("") {
@Override
public boolean apply(JavaClass input) {
return input.getPackageName().startsWith(UI_PACKAGE)
&& !input.getPackageName().startsWith(UI_OUTPUT_PACKAGE);
}
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

wld like to point this out, here we are letting e2e tests access the output, as its needed to do assertions (the old way with datastore was to convert the Data to the actual DTO, but that cannot be done now as the Data does not actually contain all the required fields to create the entity (this is due to SQL being relational, e.g. return FeedbackResponseData, we do not actually have the full course to create the entity)

see AbstractBackDoor.java for examples of how this Data -> Entity is done in datastore, and i think its not possible for our new SQL setup

Copy link
Contributor

@ziqing26 ziqing26 left a comment

Choose a reason for hiding this comment

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

LGTM!

Comment on lines +111 to +113
public void verifyNotificationTab(Notification notification, Set<String> readNotificationIds) {
boolean isRead = readNotificationIds.contains(notification.getId().toString());
WebElement notificationTab = notificationTabs.findElement(By.id(notification.getId().toString()));
Copy link
Contributor

Choose a reason for hiding this comment

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

This is much cleaner than putting id in the title, thanks for the change

Copy link
Contributor

@weiquu weiquu left a comment

Choose a reason for hiding this comment

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

LGTM

@weiquu weiquu changed the base branch from v9-migration to master February 24, 2024 07:00
@weiquu weiquu merged commit affcc46 into TEAMMATES:master Feb 24, 2024
6 of 9 checks passed
@FergusMok FergusMok added the c.Task Other non-user-facing works, e.g. refactoring, adding tests label Mar 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c.Task Other non-user-facing works, e.g. refactoring, adding tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants