-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
[#9536] Instructor Feedback Sessions Page E2E Test #10521
Conversation
…instructor-sessions-e2e # Conflicts: # src/web/app/components/session-edit-form/session-edit-form.component.html
…instructor-sessions-e2e # Conflicts: # src/web/app/components/session-edit-form/session-edit-form.component.html # src/web/app/components/sessions-recycle-bin-table/sessions-recycle-bin-table.component.html # src/web/app/pages-instructor/instructor-sessions-page/instructor-sessions-page.component.html
src/e2e/java/teammates/e2e/pageobjects/InstructorFeedbackSessionsPage.java
Outdated
Show resolved
Hide resolved
…instructor-sessions-e2e # Conflicts: # src/e2e/java/teammates/e2e/pageobjects/AppPage.java # src/e2e/java/teammates/e2e/util/BackDoor.java # src/e2e/resources/testng-e2e.xml # src/test/java/teammates/test/cases/BaseTestCaseWithDatastoreAccess.java # src/web/app/components/session-edit-form/session-edit-form.component.html
8efcef6
to
c143bd6
Compare
try { | ||
String expectedFilePath = getTestResultsFolder() + expectedFileName; | ||
String actualFilePath = getTestDownloadsFolder() + actualFileName; | ||
assertEquals(FileHelper.readFile(expectedFilePath), FileHelper.readFile(actualFilePath)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works, but we don't really need to do a full check (otherwise we need to also maintain these test files). Just doing a quick sanity check (file name and maybe a few representative lines) is good enough.
7e459e2
to
81e213a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done in general, some changes suggested.
while (!actual && retryLimit > 0) { | ||
retryLimit--; | ||
ThreadHelper.waitFor(1000); | ||
actual = emailAccount.isEmailWithSubjectPresent(subject); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have seen a lot of this kind of operation, and it's not just on this PR. Is it possible to build some kind of abstraction for them? Or perhaps use the RetryManager
class? But don't need to do on this PR.
|
||
______TS("download results"); | ||
feedbackSessionsPage.downloadResults(openSession); | ||
ThreadHelper.waitFor(5000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use retries here, where the success condition is the existence of downloaded file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added it to the verifyDownloadedFile
method
.stream() | ||
.filter(fs -> fs.getFeedbackSessionName().equals(feedbackSessionName)) | ||
.findFirst() | ||
.orElse(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is because I realized it is the only method with a different indentation level. Should I not include it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should normally count as unrelated change, but I can close one eye.
|
||
if (feedbackSession == null) { | ||
return new JsonResult("No feedback session with name " + feedbackSessionName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
protected void verifyDownloadedFile(String expectedContent, String expectedFileName) { | ||
try { | ||
String filePath = getTestDownloadsFolder() + expectedFileName; | ||
assertTrue(FileHelper.readFile(filePath).contains(expectedContent)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, just checking one line may not be enough. Let's make expectedContent
a list.
…instructor-sessions-e2e # Conflicts: # src/web/app/components/sessions-table/__snapshots__/sessions-table.component.spec.ts.snap # src/web/app/components/sessions-table/sessions-table.component.html # src/web/app/pages-instructor/instructor-home-page/__snapshots__/instructor-home-page.component.spec.ts.snap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
.stream() | ||
.filter(fs -> fs.getFeedbackSessionName().equals(feedbackSessionName)) | ||
.findFirst() | ||
.orElse(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should normally count as unrelated change, but I can close one eye.
Part of #9536
Outline of Solution
BaseE2ETestCase
to verify downloaded files existBaseE2ETestCase
to compare emails sentInstructorFeedbackSessionsPage
to represent instructor sessions pageInstructorFeedbackSessionsPageE2ETest
to test: