InstructorCourseDetailsPageUiTest: testing on live server: increase success rate for checking email delivery #7788 #8862

tshradheya · 2018-06-01T06:22:55Z

Outline of Solution

Used retry manager instead of constant ThreadHelper.wait(5000)
I haven't tested on live server.

tshradheya · 2018-06-01T06:23:43Z

src/test/java/teammates/test/cases/browsertests/InstructorCourseDetailsPageUiTest.java

-        String keyReceivedInEmail =
-                EmailAccount.getRegistrationKeyFromGmail(studentEmail, courseName, courseId);
-        return keyToSend.equals(keyReceivedInEmail);
+        RetryManager RM = new RetryManager(3);


Unsure if I should still keep this as 5 seconds

Let's keep it at 5 seconds. Part of the reason RetryManager wasn't used at the time was that it only supports exponential backoff. However, emails can take few seconds to a minute to deliver. Using exponential backoff can cause the wait time to explode.

Ideally, we should eventually improve RetryManager to be more flexible, i.e. allow for linear to exponential backoff.

In any case, it's probably still better to make a single test retry (albeit slowly) than to restart the entire test.

So I suppose there's a separate problem of our UI tests requiring that many tests require on the previous state of the other.

So I guess I would just talk about it here:
Making individual UI tests depend on the state of the previous is not a good idea but we did it anyway to reduce the time for testing.

However, the problems it caused is worse:

Developers either have to read the entire UI test class to modify tests properly

Or just read a small part (risky!) and make a change

And because of the second point which is common, it has caused a number of problems in tests and sometimes the unexpected previous state is problematic.

If a developer modify a test before another, it changes the previous state for many other tests that depend on it, which usually don't cause problems but sometimes it breaks other tests but what's worse is it sometimes hides(!!) a problem in another test.

It would be a lot of work to decouple the states and it would probably increase test time by a lot but I think it is probably what we should aim for. Eventually, as we start to be able to create more unit and integration tests, we can get rid of many tests that should not be testing at the UI level. I think we should also use an automated testing platform like Sauce Labs (Not sure if we are applicable for their open source license though) and it can really help in debugging UI tests.

RIght now if we cannot reproduce the UI tests locally, on AppVeyor we have to create a remote session to view the UI test and on Travis we don't even have a GUI so we probably have to resort to different heuristics like generating HTML files and making educated guesses.

Also we may want to use Sauce Labs to run cross browser tests, which is better because we sometimes write tests that are flaky only because they are not written properly but if we enforce that tests must work across browsers it can encourage writing less flaky tests.

LiHaoTan

Is there a way to use RetryableTaskReturns<String> instead?

What we want to retrieve is keyReceivedInEmail and not a boolean?

Also, can we change listMessagesResponse = service.users().messages().list(username).setMaxResults(5L) .setQ("is:UNREAD").execute();

to not just retrieve the last 5 emails only?

LiHaoTan · 2018-06-02T04:37:27Z

src/test/java/teammates/test/cases/browsertests/InstructorCourseDetailsPageUiTest.java

+            @Override
+            public Boolean run() throws RuntimeException {
+
+                keyToSend = BackDoor.getEncryptedKeyForStudent(courseId, studentEmail);


Isn't this constant? Why is there a need to put this in run()?

@LiHaoTan If not just last 5 then how many?

Let's fetch everything?

LiHaoTan

We shouldn't handle exceptions like that.

LiHaoTan · 2018-06-10T04:30:24Z

src/test/java/teammates/test/cases/browsertests/InstructorCourseDetailsPageUiTest.java

-                EmailAccount.getRegistrationKeyFromGmail(studentEmail, courseName, courseId);
+        RetryManager retryManager = new RetryManager(5);
+
+        String keyReceivedInEmail = retryManager.runUntilSuccessful(new RetryableTaskReturns<String>("Received email") {


Retrieve registration key is more accurate?

LiHaoTan · 2018-06-10T04:33:32Z

src/test/java/teammates/test/cases/browsertests/InstructorCourseDetailsPageUiTest.java

-        ThreadHelper.waitFor(5000); //TODO: remove this and use RetryManager with a shorter wait interval
-        String keyReceivedInEmail =
-                EmailAccount.getRegistrationKeyFromGmail(studentEmail, courseName, courseId);
+        RetryManager retryManager = new RetryManager(5);


Let's add a // TODO: Use linear backoff first before exponential backoff.

LiHaoTan · 2018-06-10T04:38:22Z

src/test/java/teammates/test/cases/browsertests/InstructorCourseDetailsPageUiTest.java

+                    keyReceivedInEmail =
+                            EmailAccount.getRegistrationKeyFromGmail(studentEmail, courseName, courseId);
+                    isDoneExecuting = true;
+                } catch (Exception e) {


We shouldn't catch all the exceptions like that. Also, these exceptions are unexpected, which means there's some kind of problem but we can't do anything useful so we shouldn't be catching them.

Returning EmailAccount.getRegistrationKeyFromGmail(studentEmail, courseName, courseId); is enough.

LiHaoTan · 2018-06-10T04:42:42Z

src/test/java/teammates/test/cases/browsertests/InstructorCourseDetailsPageUiTest.java

+            boolean isDoneExecuting;
+
+            @Override
+            public String run() throws RuntimeException {


Let's throw the specific exceptions here.

This isn't possible cause run() is an inherited method and changing definition function will cause many irrelevant changes in other places.

I have implemented in a different way.

We can use RetryableTaskReturnsThrows?

LiHaoTan · 2018-06-10T04:43:57Z

src/test/java/teammates/test/cases/browsertests/InstructorCourseDetailsPageUiTest.java

+
+            @Override
+            public boolean isSuccessful(String result) {
+                return isDoneExecuting;


The condition should just be result != null.

LiHaoTan · 2018-06-10T04:55:29Z

src/test/java/teammates/test/cases/browsertests/InstructorCourseDetailsPageUiTest.java

@@ -269,12 +270,39 @@ private InstructorCourseDetailsPage getCourseDetailsPage() {

    private boolean hasStudentReceivedReminder(String courseName, String courseId, String studentEmail)
            throws Exception {


Let's throw the specific exceptions here.

LiHaoTan · 2018-06-10T05:17:43Z

src/test/java/teammates/test/driver/EmailAccount.java

-                // unexpectedly if there are 5 additional emails received excluding the one from TEAMMATES.
-                listMessagesResponse = service.users().messages().list(username).setMaxResults(5L)
-                        .setQ("is:UNREAD").execute();
+                // Fetch all emails received by user.


This comment is not very useful because it is just repeating what the following code is doing.

I suppose the problem is that each method called should be on a separate line and getRegistrationKeyFromGmail will be better as non-static.

But EmailAccount.java is a utility class, so making it non-static doesn't make sense

Except that EmailAccount has state such as username which while immutable is used in many parts of the code.

LiHaoTan

Just one issue

LiHaoTan · 2018-07-01T09:33:12Z

src/test/java/teammates/test/cases/browsertests/InstructorCourseDetailsPageUiTest.java

@@ -272,24 +271,18 @@ private InstructorCourseDetailsPage getCourseDetailsPage() {
    }

    private boolean hasStudentReceivedReminder(String courseName, String courseId, String studentEmail)
-            throws MaximumRetriesExceededException {
+            throws Throwable {


Use Exception? TEAMMATES is kinda stuck with it because of some not so good design of mixing exceptions with errors. Some errors in Java are pretty much unrecoverable and we can't do anything useful with it such as OutOfMemmoryError.

tshradheya · 2018-07-02T02:57:25Z

@LiHaoTan, Made necessary changes
Ready for review

Use retry manager

5917244

tshradheya added the s.ToReview The PR is waiting for review(s) label Jun 1, 2018

tshradheya commented Jun 1, 2018

View reviewed changes

tshradheya requested a review from LiHaoTan June 1, 2018 06:23

Code quality corrections

f06df6f

LiHaoTan requested changes Jun 2, 2018

View reviewed changes

LiHaoTan self-assigned this Jun 3, 2018

LiHaoTan added s.Ongoing The PR is being worked on by the author(s) and removed s.ToReview The PR is waiting for review(s) labels Jun 3, 2018

tshradheya added 2 commits June 4, 2018 09:28

Update return type

eb11d64

Fetch all emails

2903bb4

tshradheya 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 Jun 4, 2018

tshradheya requested a review from LiHaoTan June 4, 2018 08:41

LiHaoTan requested changes Jun 10, 2018

View reviewed changes

LiHaoTan added s.Ongoing The PR is being worked on by the author(s) and removed s.ToReview The PR is waiting for review(s) labels Jun 10, 2018

tshradheya added 5 commits June 11, 2018 10:55

Make requested changes

1f88cd0

Workaround

f9f3772

Fix indentation

8b8d3a9

Refactor

b8633a1

Fix indentation

934f3c5

tshradheya 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 Jun 25, 2018

tshradheya requested a review from LiHaoTan June 25, 2018 04:16

LiHaoTan requested changes Jul 1, 2018

View reviewed changes

tshradheya added 2 commits July 2, 2018 10:53

Merge branch 'master' into 7788-use-retry-manager

bd91c2d

Use exception instead of Throwable

9b2ba77

tshradheya requested a review from LiHaoTan July 2, 2018 02:57

LiHaoTan added s.FinalReview The PR is ready for final review and removed s.ToReview The PR is waiting for review(s) labels Jul 2, 2018

LiHaoTan approved these changes Jul 2, 2018

View reviewed changes

LiHaoTan requested a review from damithc July 2, 2018 04:15

damithc approved these changes Jul 2, 2018

View reviewed changes

damithc 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 Jul 2, 2018

Merge branch 'master' into 7788-use-retry-manager

8f089ce

tshradheya merged commit 444c092 into TEAMMATES:master Jul 2, 2018

tshradheya added the e.2 label Jul 2, 2018

tshradheya added this to the V6.7.0 milestone Jul 2, 2018

xpdavid added the c.Task Other non-user-facing works, e.g. refactoring, adding tests label Jul 8, 2018

tshradheya mentioned this pull request Jul 12, 2018

InstructorCourseDetailsPageUiTest.testRemindAction failing on live server #8957

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

InstructorCourseDetailsPageUiTest: testing on live server: increase success rate for checking email delivery #7788 #8862

InstructorCourseDetailsPageUiTest: testing on live server: increase success rate for checking email delivery #7788 #8862

tshradheya commented Jun 1, 2018

tshradheya Jun 1, 2018

LiHaoTan Jun 2, 2018 •

edited

Loading

LiHaoTan left a comment •

edited

Loading

LiHaoTan Jun 2, 2018

tshradheya Jun 4, 2018

LiHaoTan Jun 4, 2018

LiHaoTan left a comment

LiHaoTan Jun 10, 2018

LiHaoTan Jun 10, 2018

LiHaoTan Jun 10, 2018

LiHaoTan Jun 10, 2018

tshradheya Jun 11, 2018 •

edited

Loading

LiHaoTan Jun 19, 2018

LiHaoTan Jun 10, 2018

LiHaoTan Jun 10, 2018

LiHaoTan Jun 10, 2018

tshradheya Jun 11, 2018

LiHaoTan Jun 19, 2018

LiHaoTan left a comment

LiHaoTan Jul 1, 2018 •

edited

Loading

tshradheya commented Jul 2, 2018

		@@ -269,12 +270,39 @@ private InstructorCourseDetailsPage getCourseDetailsPage() {

		private boolean hasStudentReceivedReminder(String courseName, String courseId, String studentEmail)
		throws Exception {

InstructorCourseDetailsPageUiTest: testing on live server: increase success rate for checking email delivery #7788 #8862

InstructorCourseDetailsPageUiTest: testing on live server: increase success rate for checking email delivery #7788 #8862

Conversation

tshradheya commented Jun 1, 2018

Choose a reason for hiding this comment

LiHaoTan Jun 2, 2018 • edited Loading

Choose a reason for hiding this comment

LiHaoTan left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LiHaoTan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tshradheya Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LiHaoTan left a comment

Choose a reason for hiding this comment

LiHaoTan Jul 1, 2018 • edited Loading

Choose a reason for hiding this comment

tshradheya commented Jul 2, 2018

LiHaoTan Jun 2, 2018 •

edited

Loading

LiHaoTan left a comment •

edited

Loading

tshradheya Jun 11, 2018 •

edited

Loading

LiHaoTan Jul 1, 2018 •

edited

Loading