Skip to content
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

FE: refactor Testcase files #1774

Open
3 tasks
juzhiyuan opened this issue Apr 15, 2021 · 22 comments
Open
3 tasks

FE: refactor Testcase files #1774

juzhiyuan opened this issue Apr 15, 2021 · 22 comments
Labels
enhancement New feature or request frontend

Comments

@juzhiyuan
Copy link
Member

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 juzhiyuan added enhancement New feature or request frontend labels Apr 15, 2021
@juzhiyuan juzhiyuan added this to the 2.6 milestone Apr 15, 2021
@nic-chen nic-chen modified the milestones: 2.6, 2.7 Apr 19, 2021
@juzhiyuan
Copy link
Member Author

@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
Copy link
Contributor

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

@guoqqqi
Copy link
Member

guoqqqi commented May 5, 2021

OK, I will assist him with the first question.

@juzhiyuan
Copy link
Member Author

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

@juzhiyuan
Copy link
Member Author

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

@iamayushdas
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Screenshot from 2021-05-07 12-27-14

@iamayushdas
Copy link
Contributor

@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
Copy link
Contributor

iamayushdas commented May 10, 2021

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

@spacewander
Copy link
Member

@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
Copy link
Contributor

@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
Copy link
Member

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
Copy link
Member

@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
Copy link
Contributor

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

@juzhiyuan
Copy link
Member Author

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
Copy link
Contributor

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 👍🏻

@juzhiyuan juzhiyuan changed the title improve Testcases FE: refactor Testcase files May 16, 2021
@liuxiran liuxiran removed this from the 2.7 milestone Aug 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request frontend
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants