Cypress tests #69
Cypress tests #69
Conversation
kerberizer
force-pushed
the
cypress-tests
branch
from
6e4e4e2 to
5112191
Compare
There was a problem hiding this comment.
@sergesoroka thank you very much for working on this. I haven't gone through all the files, because some of my comments concern the overall direction and we might want to completely rework the pull request. So let's start the discussion.
cypress.config.js
Outdated
| // implement node event listeners here | ||
| on("before:browser:launch", (browser = {}, launchOptions) => { | ||
| if (browser.name === "chrome") { | ||
| launchOptions.args.push("--disable-web-security"); |
There was a problem hiding this comment.
Since we're using fixtures, do we really care about CORS?
cypress.config.js
Outdated
| if (browser.name === "chrome") { | ||
| launchOptions.args.push("--disable-web-security"); | ||
| launchOptions.args.push( | ||
| "--disable-features=IsolateOrigins,site-per-process" |
cypress.config.js
Outdated
| }, | ||
| }, | ||
| fixturesFolder: "cypress/fixtures", |
There was a problem hiding this comment.
According to the documentation this is the default. Do we need to set it explicitly?
There was a problem hiding this comment.
You're right. Removed.
cypress/e2e/basic.cy.js
Outdated
| fixture: 'json/bk_rcapi_template_default.json', | ||
| }) | ||
| cy.visit(testURLRoot) | ||
| const testURLRoot = "http://localhost:5173/templates"; |
There was a problem hiding this comment.
Please don't use vite for testing. vite preview is better (it listens on port 4173), because it compiles everything and then serves it via a web server, but I feel that completely independent solution like http-server (which is set to listen on 50722) is even better. Ideally, we would test with Nginx so that our test setup is as close to production as possible, but this would make local testing somewhat more complicated. This is why I thought http-server was a good middle ground.
To run the test locally:
npm run build-servein one terminalnpx cypress runin another
As for localhost vs 127.0.0.1, the former can be resolved to ::1 (IPv6) and I've found that this can sometimes cause problems on some setups. Because http-server is told explicitly to listen on 127.0.0.1, setting it in the tests the same way helps to avoid such problems.
There was a problem hiding this comment.
It seems to be very important. Thank you for explaining me.
cypress/e2e/basic.cy.js
Outdated
| expect(request.body).to.include("template_name"); | ||
| expect(request.body).to.include("template_author"); | ||
| expect(request.body).to.include("template_acknowledgment"); | ||
| }); |
There was a problem hiding this comment.
Same as above. I'm under the impression—which could be wrong—that we're testing what the backend will return. But we aren't testing the backend. That's why we want fixtures, so the backend could be completely eliminated from the tests, emulating as if it always returns correct (and expected) responses. This allows us to test only the functionality of the frontend we're developing here. Again, I apologize if I'm stupidly missing something obvious.
There was a problem hiding this comment.
You're right. I'll remove these checkings
package.json
Outdated
| @@ -36,6 +36,7 @@ | |||
| "zustand": "^4.5.0" | |||
| }, | |||
| "devDependencies": { | |||
| "@types/cypress": "^1.1.6", | |||
There was a problem hiding this comment.
Do we really need this? https://www.npmjs.com/package/@types/cypress says it's deprecated and included in cypress itself.
There was a problem hiding this comment.
@sergesoroka Could you please check this? (from previous review)
src/App.cy.jsx
Outdated
| // expect(interception.response.statusCode).to.eq(200); | ||
| // }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
I guess this was added here by the Cypress' "Create from component" tool. I don't know why Cypress does this, but tests definitely not be added to our application source. The proper location, I believe, is cypress/component.
I have the feeling that we're not comfortable yet with how component testing works, so maybe we should skip it for now? It would help debugging problems, but our priority right now is the user experience, which E2E tests.
May I suggest we split component testing in possibly a separate branch and pull request?
There was a problem hiding this comment.
May I am mistaken but Nina insisted of using Component tests as well. She mentioned TDD (Test Driven Development), so I started writing these tests. As I saw it earlier in React app it common place to put a test beside a component itself, but of course it would be seperate directory e. g. tests. And we can use separate branch, too.
There was a problem hiding this comment.
Regarding the test files location, apparently there's a concept of collocation, which puts unit tests next to the components they test. I'll need to read more about it to form an opinion. It might actually be a good opportunity to review our directory structure (also for spectrasearch perhaps).
As for TDD, I believe @vedina primarily meant E2E tests. The rationale is that this is mostly a user interface application that does—for now at least—minimal data processing. Our main workflows that we need to test—and to design our application around—are these user interactions. It would be different for a library where the implemented algorithms would be the main functionality and E2E testing might not even make sense.
Of course, this doesn't mean that component testing is not desired. My point is that it is only secondary and mostly helpful to ourselves: proper component testing might help us determine why an E2E test fails (and, on rare occasions, might even catch some problems that weren't evident from the existing E2E tests).
To summarize, I still think it is best to separate the component tests in a different branch. It would be better even just for the review process.
There was a problem hiding this comment.
Ok, let's focus on E2E tests for now.
| "template_author": "Sergey", | ||
| "template_acknowledgment": "Test" | ||
| } | ||
| ] |
There was a problem hiding this comment.
Isn't it better to keep a real life example here? I think in the future we might also want to add different examples representing different edge cases. But I'm not sure I understand what is the advantage of this change.
There was a problem hiding this comment.
@sergesoroka Let's keep only entries that were created by us, e.g., Nina, Geri, Nick, etc. If two entries are enough for testing, we can use the file I've put there. If more that two entries are needed, let's add something entered by our people. I'm worried about having other people's data in our tests.
tsconfig.json
Outdated
| @@ -4,6 +4,7 @@ | |||
| "baseUrl": ".", | |||
| "paths": { | |||
| "@/*": ["./src/*"] | |||
| } | |||
| }, | |||
| "esModuleInterop": true | |||
There was a problem hiding this comment.
@sergesoroka Could you please see this? (from previous review)
kerberizer
force-pushed
the
cypress-tests
branch
from
5112191 to
64d2a0c
Compare
cypress/e2e/basic.cy.js
Outdated
|
|
||
| cy.intercept("GET", "/template/*", { | ||
| fixture: "json/bk_rcapi_template_default.json", | ||
| }).as("getTemplate"); |
There was a problem hiding this comment.
@sergesoroka Do we really need this when we have an intercept defined with beforeEach() above?
cypress/e2e/basic.cy.js
Outdated
|
|
||
| cy.intercept("POST", "/template/*", { | ||
| fixture: "json/bk_rcapi_template_default.json", | ||
| }).as("saveTemplate"); |
There was a problem hiding this comment.
@sergesoroka Shouldn't this be a different JSON? We probably need to check what the backend returns on a POST.
| "template_author": "Sergey", | ||
| "template_acknowledgment": "Test" | ||
| } | ||
| ] |
There was a problem hiding this comment.
@sergesoroka Let's keep only entries that were created by us, e.g., Nina, Geri, Nick, etc. If two entries are enough for testing, we can use the file I've put there. If more that two entries are needed, let's add something entered by our people. I'm worried about having other people's data in our tests.
package.json
Outdated
| @@ -36,6 +36,7 @@ | |||
| "zustand": "^4.5.0" | |||
| }, | |||
| "devDependencies": { | |||
| "@types/cypress": "^1.1.6", | |||
There was a problem hiding this comment.
@sergesoroka Could you please check this? (from previous review)
tsconfig.json
Outdated
| @@ -4,6 +4,7 @@ | |||
| "baseUrl": ".", | |||
| "paths": { | |||
| "@/*": ["./src/*"] | |||
| } | |||
| }, | |||
| "esModuleInterop": true | |||
There was a problem hiding this comment.
@sergesoroka Could you please see this? (from previous review)
src/ui/Button.cy.tsx
Outdated
| cy.mount(<Button label="View" disabled={false} />); | ||
| cy.get("[data-cy=View]").should("have.text", "View"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
@sergesoroka This component test still seems to be around.
There was a problem hiding this comment.
removed as well
src/ui/Select.cy.tsx
Outdated
| // @ts-ignore | ||
| cy.mount(<Select url={url} projectName="" />); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
removed as well
There was a problem hiding this comment.
@sergesoroka I think we're gradually getting in good shape with this, though it also becomes evident that we still have a lot of work even with E2E testing alone.
I've taken the liberty to reorganize the tests. Let's keep them for now in a single file.
We'll branch individual test files from it once the number of tests becomes sufficiently large. For example, once/if we have several tests for sorting, we'll put them in a separate file. Let's agree on having at least 3 tests in a specific area (e.g., sorting, searching, editing) before we put them in a separate file. The rest of the tests will stay in general.
Here are a few important points in my view:
- Let's focus on completing the existing tests before adding new ones.
- Make sure that each test is indeed complete. For example, when we test the search functionality, let's make sure that we're actually getting the expected result—and only it (e.g., if we know we should get two templates matching the search string, make sure we have these two templates found and not more). Similarly, let's check that sorting does actually produce sorted results.
- Sorting perhaps calls for further optimization of our fixtures. I think it might be best to not use real data at all, but instead create by hand a list of convenient templates for the fixture that would facilitate testing. For example, we could have templates named A, B, C, D, etc., that would make sorting tests easier. We could also have templates named
String1 String2,String1 String3,String2 String3, which would help with testing the search functionality. We could even have different fixtures for different tests—or we could create a single one, like we use now, that would be suitable for all tests.
Last but not least, there are quite a few files in src that are changed in this branch. Are these changes connected to the Cypress tests? If they are not, maybe we should put them in a separate pull request?
Sorry for being so slow with my reviews. I'll try to increase my pace here.
| it("can open an existing draft for editing", () => { | ||
| cy.get('[data-cy="draft"]').click(); | ||
| cy.get(".nonSelected td").eq(1).click(); | ||
| cy.get('[data-cy="Edit blueprint"]').click(); |
There was a problem hiding this comment.
@sergesoroka This tests seems to be destined to fail without a proper intercept that would load the existing draft data from a fixture rather than from the non existing backend. I'm not sure how it seemed to work before the test files reorganization.
TL;DR: To properly test this, I believe we should have a proper intercept/fixture for the specific GET to /template/{UUID}.
| cy.get(".nonSelected td").eq(1).click(); | ||
| cy.get('[data-cy="Edit blueprint"]').click(); | ||
|
|
||
| cy.get("#Save").click(); |
There was a problem hiding this comment.
@sergesoroka Why do we need to click "Save" here? It's not a bad idea to test saving, but to be meaningful, we should intercept the POST request that we'll make and check its payload for the expected JSON.
| it("can find an existing draft by searching", () => { | ||
| cy.get(".search").click().type("hts_metadata_test_finalized"); | ||
| cy.get(".nonSelected td").eq(1).click(); | ||
| }); |
There was a problem hiding this comment.
@sergesoroka Are we certain here that we're getting the expected search result?
| cy.get('[data-cy="Acknowledgment"]').click(); | ||
| cy.get('[data-cy="Acknowledgment"]').click(); | ||
| cy.get('[data-cy="Acknowledgment"]').click(); | ||
| }); |
There was a problem hiding this comment.
@sergesoroka Being able to click the column header is a good start, but we definitely want to check if the sorting actually produces the expected result too.
|
@sergesoroka Do you think we might need a meeting to discuss the next steps? |
|
that's a right idea to optimize the fixture so we can use different fixtures for different tests |
yes, I've added data attribute (data-cy=element) to select html elements for testing |
of course, let's connect when you have time |
There was a problem hiding this comment.
@sergesoroka Thanks again! I really like the script. This isn't a full review, but we have been doing it incrementally so far anyway.
| random_status = random.choice(['DRAFT', 'FINALIZED']) | ||
|
|
||
| template = { | ||
| "uri": f"https://api-test.ramanchada.ideaconsult.net/template/{random_string(21)}", |
There was a problem hiding this comment.
@sergesoroka Let's put the UUID here, just like in the real templates. Something like:
uuid = str(uuid.uuid4())
template = {
"uri": f"https://api-test.ramanchada.ideaconsult.net/template/{uuid}",
"uuid": uuid,
cypress/tools/fixture-generator.py
Outdated
| def random_date(start, end): | ||
| delta = end - start | ||
| random_days = random.randint(0, delta.days) | ||
| return start + timedelta(days=random_days) |
There was a problem hiding this comment.
@sergesoroka We're currently only testing with up to days precision. Let's tighten the random time interval to one year, e.g.:
start_date = datetime(2023, 7, 1)
end_date = datetime(2024, 7, 1)And let's add random microseconds with something like:
random_days = random.randint(0, delta.days)
random_us = random.randint(1, 86400000000) # intentionally have 1 as lower bound
return start + timedelta(days=random_days, microseconds=random_us)
cypress/tools/fixture-generator.py
Outdated
| return ''.join(random.choices(string.printable + | ||
| string.digits + | ||
| string.punctuation + | ||
| string.whitespace, k=length)) |
There was a problem hiding this comment.
@sergesoroka string.printable should be enough, because it includes the other ones.
cypress/tools/fixture-generator.py
Outdated
|
|
||
| data = [] | ||
|
|
||
| for _ in range(15): |
There was a problem hiding this comment.
Let's produce much larger number of templates. This will allow for greater variety, which might help catch more possible edge cases. And we might want to be sure that our app is working fine even with a large number of templates on the backend. How about 100-150?
cypress/tools/fixture-generator.py
Outdated
| "uuid": str(uuid.uuid4()), | ||
| "METHOD": "CFE", | ||
| "timestamp": date.isoformat(), | ||
| "PROTOCOL_CATEGORY_CODE": "TO_GENETIC_IN_VITRO_SECTION", |
There was a problem hiding this comment.
Let's find a way to randomize this properly as well.
|
@kerberizer I've updated the python script. Thanks for your comments! They are very helpful. |
Added Cypress tests: both the e2e and the Components ones to integrate into CI/CD pipeline.