Skip to content

E2e Test Fixes #25058

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

Closed
wants to merge 11 commits into from
Closed

E2e Test Fixes #25058

wants to merge 11 commits into from

Conversation

kdumontnu
Copy link
Contributor

@kdumontnu kdumontnu commented Jun 2, 2023

  • Run e2e tests in series so that DB + fixtures can reinitialize between each test
  • Run only a single test each test loop iteration 🤦‍♂️
  • Reenable firefox test (Closes Firefox E2E tests are failing with "Page closed" #21355)
  • Use locator instead of query for playwright functions

To Do:

  • Add more tests?
  • Bump playwright and review test setup

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 2, 2023
@silverwind
Copy link
Member

review test setup

should check if Firefox can be re-enabled or if it's still failing.

@pull-request-size pull-request-size bot added size/L and removed size/M labels Jun 5, 2023
@kdumontnu
Copy link
Contributor Author

review test setup

should check if Firefox can be re-enabled or if it's still failing.

I've reenabled. I've been running it on my fork for a while and haven't seen it fail yet. 👍

As far as other tests go, do you have any recommendations for pieces of the front-end we know are fragile or we've run into issues repeatedly?

@kdumontnu kdumontnu marked this pull request as ready for review June 5, 2023 21:29
@kdumontnu kdumontnu changed the title Remove DISPLAY env setting and add debug option E2e Test Fixes Jun 5, 2023
@silverwind
Copy link
Member

silverwind commented Jun 8, 2023

As far as other tests go, do you have any recommendations for pieces of the front-end we know are fragile or we've run into issues repeatedly?

Nothing really comes to mind, but if you are looking for something to test, I'd say the PR flow is something that would be nice to have, e.g.

  • Create branch
  • Edit a file in Monaco, commit it
  • Create PR
  • Review the PR on a second account
  • Merge the PR
  • Delete the branch

Part of this flow is already covered by backend integration tests, but it should really be moved to E2E instead and out of backend integration tests.

@silverwind
Copy link
Member

As far as other tests go, do you have any recommendations for pieces of the front-end we know are fragile or we've run into issues repeatedly?

Nothing really comes to mind, but if you are looking for something to test, I'd say the PR flow is something that would be nice to have, e.g.

* Create branch

* Edit a file in Monaco, commit it

* Create PR

* Review the PR on a second account

* Merge the PR

* Delete the branch

Part of this flow is already covered by backend integration tests, but it should really be moved to E2E instead and out of backend integration tests.

Maybe also start simpler:

  • Open issue
  • Comment on it
  • Close it

@kdumontnu kdumontnu requested a review from silverwind July 7, 2023 18:57
fmt.Printf("%v", stderr.String())
log.Fatal("Playwright Failed: %s", err)
} else {
fmt.Printf("%v", stdout.String())
Copy link
Member

Choose a reason for hiding this comment

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

I'm surposed ths fmt.Printf passes the linter.

@@ -0,0 +1,56 @@
// @ts-check
Copy link
Member

Choose a reason for hiding this comment

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

What's this for?

@@ -53,7 +47,7 @@ export async function save_visual(page) {
timeout: 20000,
mask: [
page.locator('.dashboard-navbar span>img.ui.avatar'),
page.locator('.ui.dropdown.jump.item span>img.ui.avatar'),
page.locator('.ui.dropdown.jump.item.tooltip span>img.ui.avatar'),
Copy link
Member

Choose a reason for hiding this comment

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

No better selector available? Maybe add a class name? This will break too easily.

await page.waitForLoadState('networkidle');
await page.locator('input#user_name').fill('user2');
await page.locator('input#password').fill('password');
await page.locator('form button.ui.green.button:visible').click();
Copy link
Member

Choose a reason for hiding this comment

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

Also a rather suboptimal selector. Afraid we have to add some class names to make tests more robust against class name changes.

await page.locator('input#email').fill(`e2e-test-${workerInfo.workerIndex}@test.com`);
await page.locator('input#password').fill('test123');
await page.locator('input#retype').fill('test123');
await page.locator('form button.ui.green.button:visible').click();
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

@wxiaoguang
Copy link
Contributor

Stale for long time.

Continue or close?

@wxiaoguang wxiaoguang added the issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail label Mar 13, 2025
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Mar 19, 2025

@kdumontnu The same question is still in my mind .... is the e2e testing system really used or useful?

After years, I can see it only consumes some CI resources but no test is really written.

So, maybe removing the e2e testing system is also a choice?

@kdumontnu
Copy link
Contributor Author

@kdumontnu The same question is still in my mind .... is the e2e testing system really used or useful?

After years, I can see it only consumes some CI resources but no test is really written.

So, maybe removing the e2e testing system is also a choice?

I think the decision must be:

  • Remove e2e tests
  • Improve and add tests

The e2e tests have certainly not been very helpful as written, which has certainly surprised me. There are some improvements that can be made to make them run more efficiently & reliably, but I don’t think that’s the major issue. Rather, it seems we’re not finding value in it.

I use them extensively in my fork to ensure I don’t regress on front-end features.

@wxiaoguang @silverwind @kerwin612 you do a lot of the front-end development recently. I think we should reflect on:

  • where do we run into the most bugs?
  • Where do we spend the most time reviewing?
  • What will help new contributors or currently prevents us from touching some code?

Some categories of “issues” I can think of are:

  • CSS leaking (changing one style and breaking a style on another page)
  • template errors, like nil de ref
  • Component initialization
  • breaking mobile layout or touch support
  • Cross-browser support
  • Performance

There are certainly different solutions to these. For instance, we could have integration tests request templates to catch 500 errors. Some of them I think are still best solved with e2e tests. I think migrating to typescript is an incredible help.

One thing I like to mention is that you are able to make contributions to the code base because you understand a lot of the nuances, but it’s difficult to scale that, which is why I suggest we focus on reviews and new contributions in the evaluation.

Happy to get everyone’s opinion though because I agree that e2e tests are not working right now

@silverwind
Copy link
Member

silverwind commented Mar 19, 2025

I use playwright testing in other applications and find those tests to be valuable to assert that the UI is working on a very basic level. log in, log out, assert certain elements are present/absent before/after login state change. I'm only also only using chrome in these tests, for the sake of having them as stable as possible.

I have not investigated how gitea's e2e tests work in detail or why they even include go code, but that go code seems superflous. e2e tests should just compile and start the app (./gitea web, with a basic .ini config and sqlite), and then run playwright suites against it.

@lunny
Copy link
Member

lunny commented Mar 19, 2025

Or maybe we can move these tests to a standalone repository.

@silverwind
Copy link
Member

Or maybe we can move these tests to a standalone repository.

Absolute no from me. Test code needs to be in the same repo as the code being tested.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Mar 26, 2025

I will answer from my side:

you do a lot of the front-end development recently. I think we should reflect on:

* where do we run into the most bugs?

Various edge cases or design problems.

* Where do we spend the most time reviewing?

Same as above, various edge cases or design problems.

* What will help new contributors or currently prevents us from touching some code?

Improve the whole framework to make code easy to write and avoid low-level mistakes.

Some categories of “issues” I can think of are:

* CSS leaking (changing one style and breaking a style on another page)

There used to be a lot, most of them are caused by hacky patches/abuses/pollution.

After recent years refactoring, the whole situation is much better now.

* template errors, like nil de ref

It could be covered by backend tests (and it is easier to be covered by backend tests because backend code could mock data more easily)

* Component initialization

I guess it is still a framework problem. At least, my new proposal "global init" will resolve most problems.

* breaking mobile layout or touch support

E2e test could do better in this field

* Cross-browser support

E2e test could do better in this field. Since we do not use cutting edge browser features, most modern browsers behave the same with our code.

* Performance

Not really related to e2e test


The really problem is:

  • If the e2e tests are written simply (simple component check, page check), backend tests and frontend unit tests could do better.
  • If the e2e tests need to be written complexly, I do not see there is manpower/time for it at current stage. The fact is nobody ever wrote a useful e2e test.

@silverwind
Copy link
Member

silverwind commented Mar 26, 2025

Keep those e2e tests as minimal as possible. Complex tests offer little value and are prone to introduce flakiness and if they are testing too specific things, they prevent progress as some maintainers may not be willing to even touch e2e tests.

I could for example see value in having e2e that just visit a few pages and assert that the browser console does not log any errors. Some recent problems would have been caught by this.

@wxiaoguang
Copy link
Contributor

I will disable the e2e test in CI (#34262) since it fails with new playwright and there is no active maintainer for it. The e2e test related code is still in code base, feel free to maintain it if anyone uses it.

@wxiaoguang
Copy link
Contributor

-> update go&js dependencies #34262

Feel free to reopen if there would be new progresses. Thank you very much!

@wxiaoguang wxiaoguang closed this Apr 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Firefox E2E tests are failing with "Page closed"
7 participants