FIX a part of #20175 : Acceptance test and WebDriverIo e2e tests Proceeds to ‘it’ Block or another 'it' block/spec Despite Failure in ‘beforeAll’ setup or any other failure in the intial specs.

Akhilesh-max commented 3 weeks ago

Overview

This PR fixes #20175 .
This PR does the following: This PR is to stop the continued execution of Acceptance tests and WebDriverIo e2e tests post the first failure.

Essential Checklist

Please follow the instructions for making a code change.

[X] I have linked the issue that this PR fixes in the "Development" section of the sidebar.
[X] I have checked the "Files Changed" tab and confirmed that the changes are what I want to make.
[X] I have written tests for my code.
[X] The PR title starts with "Fix #bugnum: " or "Fix part of #bugnum: ...", followed by a short, clear summary of the changes.
[X] I have assigned the correct reviewers to this PR (or will leave a comment with the phrase "@{{reviewer_username}} PTAL" if I can't assign them directly).

Proof that changes are correct

Before (Acceptance tests)

After (Acceptance tests)

Before (e2e Wdio tests)

https://github.com/oppia/oppia/assets/124369508/542ccb0c-a22b-4ec7-934a-1026ea7fc62b

After (e2e Wdio tests)

https://github.com/oppia/oppia/assets/124369508/be57b5c2-be18-4fb5-aaff-4fccb791391b

PR Pointers

Never force push! If you do, your PR will be closed.
To reply to reviewers, follow these instructions: https://github.com/oppia/oppia/wiki/Make-a-pull-request#step-5-address-review-comments-until-all-reviewers-approve
Some e2e tests are flaky, and can fail for reasons unrelated to your PR. We are working on fixing this, but in the meantime, if you need to restart the tests, please check the "If your build fails" wiki page.
See the Code Owner's wiki page for what code owners will expect.

oppiabot[bot] commented 3 weeks ago

Hi @Akhilesh-max please assign the required reviewer(s) for this PR. Thanks!

oppiabot[bot] commented 2 weeks ago

Hi @Akhilesh-max, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 4 days, it will be automatically closed so that others can take up the issue. If you are still working on this PR, please make a follow-up commit within 4 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

StephenYu2018 commented 1 week ago

@Akhilesh-max Why did you remove the reviewer request for @U8NWXD? I think getting feedback from someone with more expertise on concerns and practices related to Dev Workflow would be helpful.

Akhilesh-max commented 1 week ago

I have drafted the PR, as I was bit unsure of the solution, I will request them for review, once the PR is ready for review. Thanks.

Akhilesh-max commented 1 week ago

@StephenYu2018 @seanlip

I’ve configured the acceptance test to stop the suite on the first failure, even if it’s in the setup (beforeAll).

However, for e2e tests, wdio does not provide any build in way to achieve this. I referred to this issue and tried introducing framework (Jasmine) specific options in the wdio.conf.js, but it didn’t help.

I guess, could we implement a custom fail-fast mechanism in the ‘AfterEach’?`, but, would we want that? Also, I am still looking into that.

For the time being, I have updated the PR description for configuring the behavior only for acceptance test. And, will do so for the e2e test in another PR once have found a working solution. Thanks.

Interestingly, in wdio tests, we can configure to stop executing all other suits, once a fixed number of suits fails. I guess, we may want that, do we?

Akhilesh-max commented 1 week ago

@StephenYu2018 @seanlip @U8NWXD PTAL

oppiabot[bot] commented 1 week ago

Unassigning @Akhilesh-max since a re-review was requested. @Akhilesh-max, please make sure you have addressed all review comments. Thanks!

seanlip commented 1 week ago

@Akhilesh-max Thanks for the heads-up. Please file a separate issue to do this for wdio tests, and link to any conversation/docs you found on the topic and summarize our current understanding of where things are -- then let's leave that issue for later. Thanks!

Akhilesh-max commented 1 week ago

@seanlip, If I got you correct, In the 'after' screenshot, the test suite failed during the setup phase itself, which is why no specific test name is displayed (and 'suit error' is thrown, which has no name) . This is different from the 'before' screenshot, where a specific test ('it' block) failed, hence the test name is shown.

However, even after the fix, if any test fails, it does shows the name. When the first test in the suit fails.

when the later ones fail.

Additionally, in wdio e2e tests, introducing jasmine specific options, works actually. Have addressed that as well. Updated the PR description with the before and after of the implementation.

PTAL.

oppiabot[bot] commented 1 week ago

Unassigning @Akhilesh-max since a re-review was requested. @Akhilesh-max, please make sure you have addressed all review comments. Thanks!

oppiabot[bot] commented 1 week ago

Unassigning @StephenYu2018 since they have already approved the PR.

oppiabot[bot] commented 1 week ago

Unassigning @seanlip since they have already approved the PR.

oppiabot[bot] commented 1 week ago

Hi @Akhilesh-max, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks!

oppia / oppia