You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2021/02/07 10:07:20 UTC
[GitHub] [apisix-dashboard] liuxiran opened a new pull request #1451: test: make create route with upstream test cases stable
liuxiran opened a new pull request #1451:
URL: https://github.com/apache/apisix-dashboard/pull/1451
Please answer these questions before submitting a pull request
- Why submit this pull request?
- [ ] Bugfix
- [ ] New feature provided
- [ ] Improve performance
- [ ] Backport patches
- Related issues
___
### Bugfix
- Description
1. https://github.com/apache/apisix-dashboard/runs/1843264349
I reproduced this failure locally, the error occurred due to It has not changed to step two after click `Next` button in step one
Solution:
add `cy.get(this.domSelector.name).should('value', data.route_name);` to ensure edit page has ready loaded
add `{force: true}` to `Next` button click to ensure the button clicked
2. https://github.com/apache/apisix-dashboard/pull/1445/checks?check_run_id=1847652754
I reproduced this failure locally, the error occurred due to It has stop on login page after `should create an upstream` run off, I tried to merge `should enter the Route creator` `should disable Upstream input boxes after selecting an existing upstream` and `should submit custom Upstream properties successfully` into one case `should create route with upstream just created`, because it will do login before every test case runs
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] juzhiyuan commented on pull request #1451: test: make create route with upstream test cases stable
Posted by GitBox <gi...@apache.org>.
juzhiyuan commented on pull request #1451:
URL: https://github.com/apache/apisix-dashboard/pull/1451#issuecomment-774809212
ping @guoqqqi
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] guoqqqi commented on pull request #1451: test: make create route with upstream test cases stable
Posted by GitBox <gi...@apache.org>.
guoqqqi commented on pull request #1451:
URL: https://github.com/apache/apisix-dashboard/pull/1451#issuecomment-774920448
I agree, it seems that we need to use assertions more often to make the tests more stable. thanks a lot, that reminds me.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] liuxiran commented on a change in pull request #1451: test: make create route with upstream test cases stable
Posted by GitBox <gi...@apache.org>.
liuxiran commented on a change in pull request #1451:
URL: https://github.com/apache/apisix-dashboard/pull/1451#discussion_r571742709
##########
File path: web/cypress/integration/route/create-route-with-upstream.spec.js
##########
@@ -46,32 +47,27 @@ context('Create Route with Upstream', () => {
cy.contains('Submit').click();
});
- it('should enter the Route creator', function () {
+ it('should create route with upstream just created', function () {
Review comment:
While learning how to use `beforeAll`, I saw the official recommendation best practices of cypress[1],
> Anti-Pattern: Coupling multiple tests together.
Best Practice: Tests should always be able to be run independently from one another and still pass.
> Anti-Pattern: Acting like you’re writing unit tests.
Best Practice: Add multiple assertions and don’t worry about it
As There is an interdependence between those small test cases, it would be better to use more `assertions` in a merged test case.
Reference:
[1] https://docs.cypress.io/guides/references/best-practices.html#Dangling-state-is-your-friend
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] liuxiran commented on a change in pull request #1451: test: make create route with upstream test cases stable
Posted by GitBox <gi...@apache.org>.
liuxiran commented on a change in pull request #1451:
URL: https://github.com/apache/apisix-dashboard/pull/1451#discussion_r571742709
##########
File path: web/cypress/integration/route/create-route-with-upstream.spec.js
##########
@@ -46,32 +47,27 @@ context('Create Route with Upstream', () => {
cy.contains('Submit').click();
});
- it('should enter the Route creator', function () {
+ it('should create route with upstream just created', function () {
Review comment:
While learning how to use `beforeAll`, I saw the best practices of cypress,
> Anti-Pattern: Coupling multiple tests together.
Best Practice: Tests should always be able to be run independently from one another and still pass.[1]
> Anti-Pattern: Acting like you’re writing unit tests.
Best Practice: Add multiple assertions and don’t worry about it [2]
As There is an interdependence between those small test cases, it would be better to use more `assertions` in a merged test case.
Reference:
[1] https://docs.cypress.io/guides/references/best-practices.html#Having-tests-rely-on-the-state-of-previous-tests
[2] https://docs.cypress.io/guides/references/best-practices.html#Creating-%E2%80%9Ctiny%E2%80%9D-tests-with-a-single-assertion
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] liuxiran commented on a change in pull request #1451: test: make create route with upstream test cases stable
Posted by GitBox <gi...@apache.org>.
liuxiran commented on a change in pull request #1451:
URL: https://github.com/apache/apisix-dashboard/pull/1451#discussion_r571735970
##########
File path: web/cypress/integration/route/create-route-with-upstream.spec.js
##########
@@ -46,32 +47,27 @@ context('Create Route with Upstream', () => {
cy.contains('Submit').click();
});
- it('should enter the Route creator', function () {
+ it('should create route with upstream just created', function () {
Review comment:
> put those into smaller test cases because they are clearer, as for login, could we try to put Line33~Line35 under beforeAll?
Let me have a try~
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] liuxiran commented on a change in pull request #1451: test: make create route with upstream test cases stable
Posted by GitBox <gi...@apache.org>.
liuxiran commented on a change in pull request #1451:
URL: https://github.com/apache/apisix-dashboard/pull/1451#discussion_r571742709
##########
File path: web/cypress/integration/route/create-route-with-upstream.spec.js
##########
@@ -46,32 +47,27 @@ context('Create Route with Upstream', () => {
cy.contains('Submit').click();
});
- it('should enter the Route creator', function () {
+ it('should create route with upstream just created', function () {
Review comment:
While learning how to use `beforeAll`, I saw the best practices of cypress[1],
> Anti-Pattern: Coupling multiple tests together.
Best Practice: Tests should always be able to be run independently from one another and still pass.
> Anti-Pattern: Acting like you’re writing unit tests.
Best Practice: Add multiple assertions and don’t worry about it
As There is an interdependence between those small test cases, it would be better to use more `assertions` in a merged test case.
Reference:
[1] https://docs.cypress.io/guides/references/best-practices.html#Dangling-state-is-your-friend
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] juzhiyuan commented on pull request #1451: test: make create route with upstream test cases stable
Posted by GitBox <gi...@apache.org>.
juzhiyuan commented on pull request #1451:
URL: https://github.com/apache/apisix-dashboard/pull/1451#issuecomment-774651134
cc @guoqqqi to review.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] LiteSun merged pull request #1451: test: make create route with upstream test cases stable
Posted by GitBox <gi...@apache.org>.
LiteSun merged pull request #1451:
URL: https://github.com/apache/apisix-dashboard/pull/1451
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] juzhiyuan commented on a change in pull request #1451: test: make create route with upstream test cases stable
Posted by GitBox <gi...@apache.org>.
juzhiyuan commented on a change in pull request #1451:
URL: https://github.com/apache/apisix-dashboard/pull/1451#discussion_r571594946
##########
File path: web/cypress/integration/route/create-route-with-upstream.spec.js
##########
@@ -46,32 +47,27 @@ context('Create Route with Upstream', () => {
cy.contains('Submit').click();
});
- it('should enter the Route creator', function () {
+ it('should create route with upstream just created', function () {
Review comment:
put those into smaller test cases because they are clearer, as for login, could we try to put Line33~Line35 under `beforeAll`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] juzhiyuan commented on a change in pull request #1451: test: make create route with upstream test cases stable
Posted by GitBox <gi...@apache.org>.
juzhiyuan commented on a change in pull request #1451:
URL: https://github.com/apache/apisix-dashboard/pull/1451#discussion_r571773387
##########
File path: web/cypress/integration/route/create-route-with-upstream.spec.js
##########
@@ -46,32 +47,27 @@ context('Create Route with Upstream', () => {
cy.contains('Submit').click();
});
- it('should enter the Route creator', function () {
+ it('should create route with upstream just created', function () {
Review comment:
cc @LiteSun @guoqqqi to know
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org