FE: refactor Testcase files

[juzhiyuan]

Feature request

Please describe your feature

Hi, this issue aims to improve Frontend's Testcases (Cypress.io).

Describe the solution you'd like

• [ ] use data value directly instead of referring
• [ ] do we need to take some codes as repeated codes?
• [ ] check if testcases are standard according to Cypress docs.

[juzhiyuan]

@guoqqqi, could you please guide @iamayushdas to complete the first issue?

We have test cases that reference a public selector file, now we need to move them from that file to each test case file.

Example:

this.selector.xxx -> selector.xxx

[iamayushdas]

Going to do this today, will take help from @guoqqqi if he will be free

[guoqqqi]

OK, I will assist him with the first question.

[juzhiyuan]

ok, and @iamayushdas, I would prefer 1 test case file 1 PR :) That will help reviewers.

[juzhiyuan]

We could list all files here that need to get updated, and then connect PRs with this issue.

[iamayushdas]

Feature request

Please describe your feature

Hi, this issue aims to improve Frontend's Testcases (Cypress.io).

Describe the solution you'd like

• [ ] use data value directly instead of referring
• [ ] do we need to take some codes as repeated codes?
• [ ] check if testcases are standard according to Cypress docs.

also add task for adding test statement for closing notification where missing

[iamayushdas]

We could list all files here that need to get updated, and then connect PRs with this issue.

oh, i didn't watch this comment, but i have added committed reconfigured tests one by one with suitable commit message, hope its ok?

[iamayushdas]

i am not being able to refactor the cypress/integration/plugin/create-edit-delete-plugin.spec.js My PC is not able to handle that much recursive tests Anyways, now focusing on other tests Error screenshot

[liuxiran]

public selector file

Hi , do we need to keep the common seletor in the public selector file? Just like notification: '.ant-notification-notice-message' empty: '.ant-empty-normal' may used in most test files

@juzhiyuan @guoqqqi @iamayushdas

[juzhiyuan]

Hi @liuxiran, here have some cases:

1. When a newcomer wants to contribute some test cases, he/she will use Selector or Data directly in the test case file, then we have to tell him/her that there has a public file contains many repeated data.
2. When we want to drop some test cases, we have to double-check if there has useless data or selectors, this work is not enjoyable 🤔

[iamayushdas]

Hi @liuxiran, here have some cases:

1. When a newcomer wants to contribute some test cases, he/she will use Selector or Data directly in the test case file, then we have to tell him/her that there has a public file contains many repeated data.

2. When we want to drop some test cases, we have to double-check if there has useless data or selectors, this work is not enjoyable 🤔

Yes, agreed , filtering the data from files is such a pain to my eyes.

[iamayushdas]

public selector file

Hi , do we need to keep the common seletor in the public selector file? Just like notification: '.ant-notification-notice-message' empty: '.ant-empty-normal' may used in most test files

@juzhiyuan @guoqqqi @iamayushdas

Separating files will help devs for adding future tests. IMO

[liuxiran]

got all of your concerns @juzhiyuan @iamayushdas, each case has its only data and selector, It does increase flexibility, and it is more convenient for maintenance, I also agree with separeting files.

Meanwhile, there are some common selectors and data, may define in every test case file, we can consider leaving them in public files. just like some lib functions and component. As a development agreement, we can supplement the development manual to every developer.

[iamayushdas]

@guoqqqi i am done with all the tests except cypress/integration/plugin/create-edit-delete-plugin.spec.js also given the reason above, if you can refactored this test, i will be thankful to you, after that remaining test to refactor we could delete those files Thank you

[iamayushdas]

@LiteSun @nic-chen @liuxiran @imjoey @qian0817 can i have the review on these improved test cases Thank you for your time :smile:

[spacewander]

@iamayushdas Can you squash all the "chore: refactored test ..." into one PR? It is not a good idea to submit one PR for one file.

[iamayushdas]

@iamayushdas

Can you squash all the "chore: refactored test ..." into one PR? It is not a good idea to submit one PR for one file.

I have done this , as it makes it easier for review and push changes efficiently as it is separated in different PRs

[spacewander]

Someone reports that you are spamming with "chore: refactored test ..." PRs via bot. According to your previous behavior, we don't think you are doing this intendedly.

But we can't merge all your 30s PR as it will cause misjudgment:

• APISIX allow people to game PR contribution statistics instead of real features

Or even worse:

• APISIX is so active because many of its commits are "chore: refactored test ...". It doesn't have much new features though it is "active".

[spacewander]

@iamayushdas

I should apologize for criticizing your behavior. You are just doing what @juzhiyuan suggested:

I would prefer 1 test case file 1 PR

However, looks like we didn't correctly estimate the number of PRs, and caused a misunderstanding.

BTW, actually, it is not a good idea to split the PR in such size, because now we need more than 100 approvals to get the whole thing down and may cause misjudgment.

As it is our fault, let's keep those PRs.

[iamayushdas]

Next time i will be cautious about it, sorry about all blunders and mistakes i have done.

[juzhiyuan]

Hi @iamayushdas, that's not your fault, because the Review speed is slow those days, so there have many pending PRs (ready for review), this is our duty to move all those PRs forward.

cc @LiteSun @liuxiran @imjoey to review.

[iamayushdas]

Hi @iamayushdas, that's not your fault, because the Review speed is slow those days, so there have many pending PRs (ready for review), this is our duty to move all those PRs forward.

cc @LiteSun @liuxiran @imjoey to review.

Okay 👍🏻