adding additional coverage #9
Conversation
|
WalkthroughThis pull request introduces new modules for managing messages, room configurations, branding data, and assertions. It also updates the test suite with additional cases covering various operations related to authentication, booking, branding, messages, reports, and rooms. The retry configuration in Playwright has been adjusted, and some faker methods have been updated. Changes
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Files selected (23)
- README.md (1)
- lib/datafactory/message.ts (1)
- lib/datafactory/room.ts (1)
- lib/helpers/branding.ts (1)
- lib/helpers/createAssertions.ts (1)
- playwright.config.ts (1)
- tests/auth/login.post.spec.ts (1)
- tests/auth/logout.post.spec.ts (1)
- tests/auth/validate.post.spec.ts (1)
- tests/booking/booking.delete.spec.ts (1)
- tests/booking/booking.get.spec.ts (1)
- tests/booking/booking.post.spec.ts (1)
- tests/booking/booking.put.spec.ts (1)
- tests/branding/branding.spec.ts (1)
- tests/message/message.delete.spec.ts (1)
- tests/message/message.get.spec.ts (1)
- tests/message/message.post.spec.ts (1)
- tests/message/message.put.spec.ts (1)
- tests/report/report.get.spec.ts (1)
- tests/room/room.delete.spec.ts (1)
- tests/room/room.get.spec.ts (1)
- tests/room/room.post.spec.ts (1)
- tests/room/room.put.spec.ts (1)
Files ignored due to filter (4)
- thunder-client-exports/thunder-collection_Branding collection.json
- thunder-client-exports/thunder-collection_Message collection.json
- thunder-client-exports/thunder-collection_Report Collection.json
- tsconfig.json
Files skipped from review due to trivial changes (12)
- README.md
- lib/datafactory/message.ts
- lib/datafactory/room.ts
- lib/helpers/createAssertions.ts
- tests/auth/validate.post.spec.ts
- tests/booking/booking.get.spec.ts
- tests/booking/booking.put.spec.ts
- tests/branding/branding.spec.ts
- tests/message/message.get.spec.ts
- tests/report/report.get.spec.ts
- tests/room/room.delete.spec.ts
- tests/room/room.get.spec.ts
Review comments generated (14)
- Review: 10
- LGTM: 4
Tips
Chat with 
CodeRabbit Bot (@coderabbitai)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@coderabbitaiin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignoreanywhere in the PR description to pause further reviews from the bot.
| @@ -1,7 +1,7 @@ | |||
| //COVERAGE_TAG: POST /auth/login | |||
|
|
|||
| import { test, expect } from "@playwright/test"; | |||
| import { isValidDate } from "../../lib/helpers/date"; | |||
| import { isValidDate } from "@helpers/date"; | |||
There was a problem hiding this comment.
The import statement has been updated to use an absolute path instead of a relative one. This is generally a good practice as it makes the code more readable and maintainable. However, please ensure that the module resolution is correctly configured to support absolute paths.
| import { isValidDate } from "@helpers/date"; | |
| import { isValidDate } from "@helpers/date"; |
| @@ -31,6 +31,6 @@ export default defineConfig({ | |||
| ignoreHTTPSErrors: true, | |||
| trace: "on", | |||
| }, | |||
| retries: 0, | |||
| retries: 2, | |||
There was a problem hiding this comment.
Increasing the number of retries might lead to longer test execution times in case of failing tests. Make sure this is acceptable for your testing environment and consider making this value configurable so it can be adjusted based on the environment where the tests are run.
| retries: 2, | |
| retries: process.env.TEST_RETRIES || 0, |
| } from "@datafactory/booking"; | ||
| import { stringDateByDays } from "@helpers/date"; | ||
| import { createRoom } from "@datafactory/room"; |
There was a problem hiding this comment.
The changes made here are primarily related to the import statements. The developer has updated the relative paths to absolute paths or different module paths. This is a good practice as it makes the code more readable and maintainable. It also reduces the chance of errors due to incorrect relative paths. Therefore, these changes look good to me.
- } from "../../lib/datafactory/booking";
- import { stringDateByDays } from "../../lib/helpers/date";
- import { createRoom } from "../../lib/datafactory/room";
+ } from "@datafactory/booking";
+ import { stringDateByDays } from "@helpers/date";
+ import { createRoom } from "@datafactory/room";| export const updatedBranding = { | ||
| name: "Test Name", | ||
| map: { | ||
| latitude: 41.8781, | ||
| longitude: 87.6298, | ||
| }, | ||
| logoUrl: "https://media.tenor.com/KaCUHzQxVWcAAAAC/house.gif", | ||
| description: "description", | ||
| contact: { | ||
| name: "Testy McTester", | ||
| address: "100 Testing Way", | ||
| phone: "5555555555", | ||
| email: "testy@testymtesterface.com", | ||
| }, | ||
| }; |
There was a problem hiding this comment.
The updatedBranding object seems to be used for testing purposes. It's a good practice to keep test data separate from the application code. Consider moving this to a dedicated test data file.
| export const updatedBranding = { | |
| name: "Test Name", | |
| map: { | |
| latitude: 41.8781, | |
| longitude: 87.6298, | |
| }, | |
| logoUrl: "https://media.tenor.com/KaCUHzQxVWcAAAAC/house.gif", | |
| description: "description", | |
| contact: { | |
| name: "Testy McTester", | |
| address: "100 Testing Way", | |
| phone: "5555555555", | |
| email: "testy@testymtesterface.com", | |
| }, | |
| }; | |
| export const updatedBranding = { | |
| name: "Test Name", | |
| map: { | |
| latitude: 41.8781, | |
| longitude: 87.6298, | |
| }, | |
| logoUrl: "https://media.tenor.com/KaCUHzQxVWcAAAAC/house.gif", | |
| description: "description", | |
| contact: { | |
| name: "Testy McTester", | |
| address: "100 Testing Way", | |
| phone: "5555555555", | |
| email: "testy@testymctestface.com", | |
| }, | |
| }; |
| //COVERAGE_TAG: POST /room/ | ||
|
|
||
| import { createRandomRoomBody } from "@datafactory/room"; | ||
| import { createHeaders } from "@helpers/createHeaders"; | ||
| import { test, expect } from "@playwright/test"; | ||
|
|
||
| test.describe("room/ POST requests", async () => { | ||
| let authHeaders; | ||
| let updateRoomBody; | ||
|
|
||
| test.beforeEach(async ({}) => { | ||
| authHeaders = await createHeaders(); | ||
| updateRoomBody = await createRandomRoomBody(); | ||
| }); | ||
|
|
||
| test("POST /room to create a room", async ({ request }) => { | ||
| const response = await request.post(`/room/`, { | ||
| headers: authHeaders, | ||
| data: updateRoomBody, | ||
| }); | ||
|
|
||
| expect(response.status()).toBe(201); | ||
| const body = await response.json(); | ||
|
|
||
| expect(typeof body.roomid).toBe("number"); | ||
| expect(body.name).toEqual(updateRoomBody.name); | ||
| expect(body.accessible).toEqual(updateRoomBody.accessible); | ||
| expect(body.description).toEqual(updateRoomBody.description); | ||
| expect(body.features).toEqual(updateRoomBody.features); | ||
| expect(body.image).toEqual(updateRoomBody.image); | ||
| expect(body.roomName).toEqual(updateRoomBody.roomName); | ||
| expect(body.type).toEqual(updateRoomBody.type); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The test case for POST /room looks good overall. However, it would be beneficial to add a teardown step after each test to delete the room that was created. This will ensure that each test is independent and does not leave any side effects that could affect other tests.
+ test.afterEach(async ({ request }) => {
+ const deleteResponse = await request.delete(`/room/${updateRoomBody.roomid}`, {
+ headers: authHeaders,
+ });
+ expect(deleteResponse.status()).toBe(204);
+ });| test.describe("message/ POST requests", async () => { | ||
| let message = await newMessageBody(); |
There was a problem hiding this comment.
The newMessageBody() function is an asynchronous function and should not be called in the describe block. It should be called inside a test or before hook. This is because the describe block is synchronous and does not support async operations.
- test.describe("message/ POST requests", async () => {
- let message = await newMessageBody();
+ test.describe("message/ POST requests", () => {
+ let message;
+ test.beforeEach(async () => {
+ message = await newMessageBody();
+ });
tests/message/message.delete.spec.ts
Outdated
| test.describe("message/ PUT requests", async () => { | ||
| let message; | ||
| let authHeaders; | ||
|
|
||
| test.beforeEach(async ({}) => { | ||
| message = await createMessage(); | ||
| authHeaders = await createHeaders(); | ||
| }); | ||
|
|
||
| test("DELETE a message", async ({ request }) => { | ||
| const response = await request.delete(`/message/${message.messageid}`, { | ||
| headers: authHeaders, | ||
| data: "", | ||
| }); | ||
|
|
||
| expect(response.status()).toBe(202); | ||
| const body = await response.text(); | ||
| expect(body).toEqual(""); | ||
|
|
||
| //check the message again and ensure it's not available | ||
| const response2 = await request.get(`/message/${message.messageid}`); | ||
| expect(response2.status()).toBe(500); | ||
| const body2 = await response2.json(); | ||
| expect(body2.error).toEqual("Internal Server Error"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The test suite is well written and covers the necessary aspects of the DELETE operation. However, there's a minor issue with the description in line 7. It should be "message/ DELETE requests" instead of "message/ PUT requests". Here's the corrected version:
- test.describe("message/ PUT requests", async () => {
+ test.describe("message/ DELETE requests", async () => {| expect(response.status()).toBe(202); | ||
| const body = await response.text(); | ||
| expect(body).toEqual(""); | ||
|
|
||
| //check the message again and ensure it's not available | ||
| const response2 = await request.get(`/message/${message.messageid}`); | ||
| expect(response2.status()).toBe(500); | ||
| const body2 = await response2.json(); | ||
| expect(body2.error).toEqual("Internal Server Error"); |
There was a problem hiding this comment.
The assertions are correct but it would be better to add more descriptive error messages for each assertion to make it easier to debug when a test fails. Here's how you can do it:
- expect(response.status()).toBe(202);
+ expect(response.status()).toBe(202, 'Expected response status to be 202');
- expect(body).toEqual("");
+ expect(body).toEqual("", 'Expected response body to be empty');
- expect(response2.status()).toBe(500);
+ expect(response2.status()).toBe(500, 'Expected second response status to be 500');
- expect(body2.error).toEqual("Internal Server Error");
+ expect(body2.error).toEqual("Internal Server Error", 'Expected error message to be "Internal Server Error"');| //COVERAGE_TAG: PUT /message/{id}/read | ||
|
|
||
| import { createMessage } from "@datafactory/message"; | ||
| import { createHeaders } from "@helpers/createHeaders"; | ||
| import { test, expect } from "@playwright/test"; | ||
|
|
||
| test.describe("message/ PUT requests", async () => { | ||
| let message; | ||
| let authHeaders; | ||
|
|
||
| test.beforeEach(async ({}) => { | ||
| message = await createMessage(); | ||
| authHeaders = await createHeaders(); | ||
| }); | ||
|
|
||
| test("PUT a message as read", async ({ request }) => { | ||
| const response = await request.put(`/message/${message.messageid}/read`, { | ||
| headers: authHeaders, | ||
| data: "", | ||
| }); | ||
|
|
||
| expect(response.status()).toBe(202); | ||
| const body = await response.text(); | ||
| expect(body).toEqual(""); | ||
|
|
||
| //get the message again and check it's read | ||
| const response2 = await request.get("/message/"); | ||
| expect(response2.status()).toBe(200); | ||
| const body2 = await response2.json(); | ||
|
|
||
| const match = body2.messages.find((item) => item.id === message.messageid); | ||
|
|
||
| expect(match).toBeDefined(); | ||
| expect(match.read).toBe(true); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The test case seems well written and covers the necessary aspects of the PUT request. However, it would be beneficial to add error handling for the async operations. This will help in debugging if any of these operations fail.
test.beforeEach(async ({}) => {
try {
message = await createMessage();
authHeaders = await createHeaders();
} catch (error) {
console.error('Setup failed:', error);
throw error;
}
});Also, consider adding a cleanup step after each test to delete the created message. This ensures that each test runs independently and doesn't affect other tests.
test.afterEach(async ({request}) => {
try {
const response = await request.delete(`/message/${message.messageid}`, {
headers: authHeaders,
});
if(response.status() !== 200) {
console.error(`Cleanup failed: Unable to delete message with id ${message.messageid}`);
}
} catch (error) {
console.error('Cleanup failed:', error);
}
});
tests/room/room.put.spec.ts
Outdated
| //COVERAGE_TAG: PUT /room/{id} | ||
|
|
||
| import { createRoom, createRandomRoomBody } from "@datafactory/room"; | ||
| import { createHeaders } from "@helpers/createHeaders"; | ||
| import { test, expect } from "@playwright/test"; | ||
|
|
||
| test.describe("room/ PUT requests", async () => { | ||
| let room; | ||
| let roomId; | ||
| let authHeaders; | ||
| let updateRoomBody; | ||
|
|
||
| test.beforeEach(async ({}) => { | ||
| room = await createRoom("PUT", 50); | ||
| roomId = room.roomid; | ||
| authHeaders = await createHeaders(); | ||
| updateRoomBody = await createRandomRoomBody(); | ||
| }); | ||
|
|
||
| test("PUT /room to update values", async ({ request }) => { | ||
| const response = await request.put(`/room/${roomId}`, { | ||
| headers: authHeaders, | ||
| data: updateRoomBody, | ||
| }); | ||
|
|
||
| expect(response.status()).toBe(202); | ||
| const body = await response.json(); | ||
|
|
||
| expect(body.roomid).toEqual(roomId); | ||
| expect(body.name).toEqual(updateRoomBody.name); | ||
| expect(body.accessible).toEqual(updateRoomBody.accessible); | ||
| expect(body.description).toEqual(updateRoomBody.description); | ||
| expect(body.features).toEqual(updateRoomBody.features); | ||
| expect(body.image).toEqual(updateRoomBody.image); | ||
| expect(body.roomName).toEqual(updateRoomBody.roomName); | ||
| expect(body.type).toEqual(updateRoomBody.type); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The test case looks good overall, but there's a potential issue with the createRoom function. It seems like you're passing "PUT" as the first argument and 50 as the second argument. If "PUT" is supposed to be the HTTP method and 50 is supposed to be the room size, it might not make sense to pass these values when creating a room. Please ensure that the parameters passed to the createRoom function are correct.
Also, consider adding error handling for the async operations in the beforeEach block. If any of these operations fail (e.g., createRoom, createHeaders, createRandomRoomBody), the test case will continue to execute and may result in misleading failures. Wrapping these operations in a try-catch block and failing the test case in the catch block can provide more clarity when something goes wrong.
Here's an example of how you could handle errors:
test.beforeEach(async ({}) => {
try {
room = await createRoom("PUT", 50);
roomId = room.roomid;
authHeaders = await createHeaders();
updateRoomBody = await createRandomRoomBody();
} catch (error) {
test.fail(`Setup failed with error: ${error.message}`);
}
});
There was a problem hiding this comment.
Files selected (1)
- tests/message/message.delete.spec.ts (1)
Review comments generated (2)
- Review: 1
- LGTM: 1
Tips
Chat with CodeRabbit Bot (@coderabbitai)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@coderabbitaiin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignoreanywhere in the PR description to pause further reviews from the bot.
|
|
||
| test("DELETE a message", async ({ request }) => { | ||
| const response = await request.delete(`/message/${message.messageid}`, { | ||
| headers: authHeaders, | ||
| data: "", | ||
| }); | ||
|
|
||
| expect(response.status()).toBe(202); | ||
| const body = await response.text(); | ||
| expect(body).toEqual(""); | ||
|
|
||
| //check the message again and ensure it's not available | ||
| const response2 = await request.get(`/message/${message.messageid}`); | ||
| expect(response2.status()).toBe(500); | ||
| const body2 = await response2.json(); | ||
| expect(body2.error).toEqual("Internal Server Error"); | ||
| }); |
There was a problem hiding this comment.
The test case for deleting a message seems to be expecting an Internal Server Error (500) after the deletion. This is not a good practice as it can lead to confusion. After deleting a resource, trying to access it should ideally return a 404 Not Found error, indicating that the resource does not exist. If your API behaves differently, please disregard this comment. However, if it's a mistake, I suggest updating the expected status code and error message accordingly.
| test("DELETE a message", async ({ request }) => { | |
| const response = await request.delete(`/message/${message.messageid}`, { | |
| headers: authHeaders, | |
| data: "", | |
| }); | |
| expect(response.status()).toBe(202); | |
| const body = await response.text(); | |
| expect(body).toEqual(""); | |
| //check the message again and ensure it's not available | |
| const response2 = await request.get(`/message/${message.messageid}`); | |
| expect(response2.status()).toBe(500); | |
| const body2 = await response2.json(); | |
| expect(body2.error).toEqual("Internal Server Error"); | |
| }); | |
| //check the message again and ensure it's not available | |
| const response2 = await request.get(`/message/${message.messageid}`); | |
| expect(response2.status()).toBe(404); | |
| const body2 = await response2.json(); | |
| expect(body2.error).toEqual("Not Found"); |
There was a problem hiding this comment.
Files selected (3)
- lib/datafactory/room.ts (3)
- lib/helpers/roomFeatures.ts (1)
- tests/room/room.put.spec.ts (1)
Files ignored due to filter (2)
- package-lock.json
- package.json
Files skipped from review due to trivial changes (2)
- lib/datafactory/room.ts
- lib/helpers/roomFeatures.ts
Review comments generated (4)
- Review: 3
- LGTM: 1
Tips
Chat with CodeRabbit Bot (@coderabbitai)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@coderabbitaiin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignoreanywhere in the PR description to pause further reviews from the bot.
| test.describe("room/ PUT requests", async () => { | ||
| let room; | ||
| let roomId; | ||
| let authHeaders; | ||
| let updateRoomBody; | ||
|
|
||
| test.beforeEach(async ({}) => { | ||
| room = await createRoom("PUT", 50); | ||
| roomId = room.roomid; | ||
| authHeaders = await createHeaders(); | ||
| updateRoomBody = await createRandomRoomBody(); | ||
| }); |
There was a problem hiding this comment.
The beforeEach hook is used to set up the state before each test. However, it seems like you're using async functions inside this hook. While Playwright's test runner does support async hooks, it's generally a good idea to keep setup code synchronous to avoid potential issues with race conditions or timeouts. If the setup steps are inherently asynchronous (like network requests), consider moving them into the tests themselves or using a different strategy to manage state.
Also, the use of global variables (room, roomId, authHeaders, updateRoomBody) can lead to problems if tests are run in parallel or if state leaks between tests. It would be better to encapsulate these within each test case.
Here's an example of how you might refactor this:
- let room;
- let roomId;
- let authHeaders;
- let updateRoomBody;
- test.beforeEach(async ({}) => {
- room = await createRoom("PUT", 50);
- roomId = room.roomid;
- authHeaders = await createHeaders();
- updateRoomBody = await createRandomRoomBody();
- });
+ test.beforeEach(() => {
+ // Reset state before each test
+ room = null;
+ roomId = null;
+ authHeaders = null;
+ updateRoomBody = null;
+ });
test("PUT /room to update values", async ({ request }) => {
+ const room = await createRoom("PUT", 50);
+ const roomId = room.roomid;
+ const authHeaders = await createHeaders();
+ const updateRoomBody = await createRandomRoomBody();
...
});|
|
||
| test("PUT /room to update values", async ({ request }) => { | ||
| const response = await request.put(`/room/${roomId}`, { | ||
| headers: authHeaders, | ||
| data: updateRoomBody, | ||
| }); | ||
|
|
||
| expect(response.status()).toBe(202); | ||
| const body = await response.json(); | ||
|
|
||
| expect(body.roomid).toEqual(roomId); | ||
| expect(body.name).toEqual(updateRoomBody.name); | ||
| expect(body.accessible).toEqual(updateRoomBody.accessible); | ||
| expect(body.description).toEqual(updateRoomBody.description); | ||
| expect(body.features).toEqual(updateRoomBody.features); | ||
| expect(body.image).toEqual(updateRoomBody.image); | ||
| expect(body.roomName).toEqual(updateRoomBody.roomName); | ||
| expect(body.type).toEqual(updateRoomBody.type); | ||
| }); |
There was a problem hiding this comment.
In your test case, you're making a PUT request and then asserting on the response status and body. This looks fine, but there's one potential issue: you're assuming that the server will always return a 202 status code for successful operations. While this may be true for your current implementation, it's not a universal standard. A more common status code for successful updates is 200. If the server's behavior changes in the future, this test could fail even though the operation is still successful. Consider making your test more flexible by allowing any 2xx status code to indicate success.
| test("PUT /room to update values", async ({ request }) => { | |
| const response = await request.put(`/room/${roomId}`, { | |
| headers: authHeaders, | |
| data: updateRoomBody, | |
| }); | |
| expect(response.status()).toBe(202); | |
| const body = await response.json(); | |
| expect(body.roomid).toEqual(roomId); | |
| expect(body.name).toEqual(updateRoomBody.name); | |
| expect(body.accessible).toEqual(updateRoomBody.accessible); | |
| expect(body.description).toEqual(updateRoomBody.description); | |
| expect(body.features).toEqual(updateRoomBody.features); | |
| expect(body.image).toEqual(updateRoomBody.image); | |
| expect(body.roomName).toEqual(updateRoomBody.roomName); | |
| expect(body.type).toEqual(updateRoomBody.type); | |
| }); | |
| expect(response.status()).toBeGreaterThanOrEqual(200); | |
| expect(response.status()).toBeLessThan(300); |
|
|
||
| test("PUT /room to update features", async ({ request }) => { | ||
| let randomFeatures = randomRoomFeaturesCount(10); | ||
|
|
||
| // Overwrites the features array with random features | ||
| updateRoomBody.features = randomFeatures; | ||
|
|
||
| const response = await request.put(`/room/${roomId}`, { | ||
| headers: authHeaders, | ||
| data: updateRoomBody, | ||
| }); | ||
|
|
||
| expect(response.status()).toBe(202); | ||
| const body = await response.json(); | ||
|
|
||
| expect(body.roomid).toEqual(roomId); | ||
| expect(body.name).toEqual(updateRoomBody.name); | ||
| expect(body.accessible).toEqual(updateRoomBody.accessible); | ||
| expect(body.description).toEqual(updateRoomBody.description); | ||
| expect(body.features).toEqual(randomFeatures); | ||
| expect(body.image).toEqual(updateRoomBody.image); | ||
| expect(body.roomName).toEqual(updateRoomBody.roomName); | ||
| expect(body.type).toEqual(updateRoomBody.type); | ||
| }); |
There was a problem hiding this comment.
This test case is very similar to the previous one, but it specifically tests updating the room features. The code looks fine, but again, consider making your test more flexible by allowing any 2xx status code to indicate success.
| test("PUT /room to update features", async ({ request }) => { | |
| let randomFeatures = randomRoomFeaturesCount(10); | |
| // Overwrites the features array with random features | |
| updateRoomBody.features = randomFeatures; | |
| const response = await request.put(`/room/${roomId}`, { | |
| headers: authHeaders, | |
| data: updateRoomBody, | |
| }); | |
| expect(response.status()).toBe(202); | |
| const body = await response.json(); | |
| expect(body.roomid).toEqual(roomId); | |
| expect(body.name).toEqual(updateRoomBody.name); | |
| expect(body.accessible).toEqual(updateRoomBody.accessible); | |
| expect(body.description).toEqual(updateRoomBody.description); | |
| expect(body.features).toEqual(randomFeatures); | |
| expect(body.image).toEqual(updateRoomBody.image); | |
| expect(body.roomName).toEqual(updateRoomBody.roomName); | |
| expect(body.type).toEqual(updateRoomBody.type); | |
| }); | |
| expect(response.status()).toBeGreaterThanOrEqual(200); | |
| expect(response.status()).toBeLessThan(300); |
There was a problem hiding this comment.
Files selected (3)
- lib/datafactory/booking.ts (3)
- lib/datafactory/message.ts (1)
- lib/datafactory/room.ts (3)
Files skipped from review due to trivial changes (1)
- lib/datafactory/booking.ts
Review comments generated (9)
- Review: 8
- LGTM: 1
Tips
Chat with CodeRabbit Bot (@coderabbitai)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@coderabbitaiin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignoreanywhere in the PR description to pause further reviews from the bot.
| import { faker } from "@faker-js/faker"; | ||
| import { request, expect } from "@playwright/test"; | ||
|
|
||
| let url = process.env.URL || "https://automationintesting.online/"; |
There was a problem hiding this comment.
Hardcoding URLs in the code is not a good practice. It's better to store such values in configuration files or environment variables. This makes it easier to change the URL without modifying the code and allows different URLs for different environments (development, testing, production, etc.). However, you are already using an environment variable process.env.URL as a fallback which is a good practice. But, the hardcoded URL should be removed.
| let url = process.env.URL || "https://automationintesting.online/"; | |
| let url = process.env.URL; |
| export const message1Summary = { | ||
| id: 1, | ||
| name: "James Dean", | ||
| subject: "Booking enquiry", | ||
| read: false, | ||
| }; |
There was a problem hiding this comment.
The message1Summary object has a hard-coded id field. If this ID is supposed to represent a unique identifier for a message, hard-coding it may lead to issues if the ID needs to change or if multiple instances of this object are created. Consider generating unique IDs dynamically or retrieving them from a database or other data source.
| export const message1 = { | ||
| messageid: 1, | ||
| name: "James Dean", | ||
| email: "james@email.com", | ||
| phone: "01402 619211", | ||
| subject: "Booking enquiry", | ||
| description: "I would like to book a room at your place", | ||
| }; |
There was a problem hiding this comment.
Similar to the previous comment, the message1 object also has a hard-coded id field. Consider generating unique IDs dynamically or retrieving them from a database or other data source.
| export const postMessage = { | ||
| name: "string", | ||
| email: "test@test.com", | ||
| phone: "stringstringstring", | ||
| subject: "string", | ||
| description: "stringstringstringst", | ||
| }; |
There was a problem hiding this comment.
The postMessage object has hard-coded string values. If these fields are meant to represent real data, consider using more meaningful test data or generating random data for each test run. This can help catch edge cases and make your tests more robust.
| export async function createMessage() { | ||
| let message = await newMessageBody(); | ||
|
|
||
| const contextRequest = await request.newContext(); | ||
| const response = await contextRequest.post(url + "message/", { | ||
| data: message, | ||
| }); | ||
|
|
||
| expect(response.status()).toBe(201); | ||
| const body = await response.json(); | ||
| return body; | ||
| } |
There was a problem hiding this comment.
In the createMessage function, you're creating a new context for every request. This might not be necessary and could potentially slow down your tests if many requests are being made. Consider reusing the same context for multiple requests if possible.
Also, there is no error handling in case the POST request fails. Consider adding a try-catch block to handle potential errors and provide more informative error messages.
| export async function createMessage() { | |
| let message = await newMessageBody(); | |
| const contextRequest = await request.newContext(); | |
| const response = await contextRequest.post(url + "message/", { | |
| data: message, | |
| }); | |
| expect(response.status()).toBe(201); | |
| const body = await response.json(); | |
| return body; | |
| } | |
| export async function createMessage(contextRequest) { | |
| let message = await newMessageBody(); | |
| try { | |
| const response = await contextRequest.post(url + "message/", { | |
| data: message, | |
| }); | |
| expect(response.status()).toBe(201); | |
| const body = await response.json(); | |
| return body; | |
| } catch (error) { | |
| console.error(`Failed to create message: ${error}`); | |
| } | |
| } |
In the above suggestion, I've added a parameter to the createMessage function so that the same context can be reused for multiple requests. I've also added a try-catch block to handle any errors that might occur when making the POST request.
| @@ -1,6 +1,7 @@ | |||
| import { expect, request } from "@playwright/test"; | |||
| import { faker } from "@faker-js/faker"; | |||
| import { createHeaders } from "../helpers/createHeaders"; | |||
| import { randomRoomFeaturesCount } from "@helpers/roomFeatures"; | |||
There was a problem hiding this comment.
The import statement for randomRoomFeaturesCount is using an absolute path. This could potentially cause issues if the directory structure changes in the future. It's generally a good practice to use relative paths for local modules.
| import { randomRoomFeaturesCount } from "@helpers/roomFeatures"; | |
| import { randomRoomFeaturesCount } from "../helpers/roomFeatures"; |
| let features = randomRoomFeaturesCount(6); | ||
|
|
||
| let roomBody = { | ||
| roomName: roomName || faker.random.numeric(3), | ||
| roomName: roomName || faker.string.numeric(3), | ||
| type: roomType[Math.floor(Math.random() * roomType.length)], // returns a random value from the array | ||
| accessible: Math.random() < 0.5, //returns true or false | ||
| image: faker.image.imageUrl(500, 500, "cat", true), | ||
| image: faker.image.urlLoremFlickr({ | ||
| category: "cat", | ||
| width: 500, | ||
| height: 500, | ||
| }), | ||
| description: faker.hacker.phrase(), | ||
| features: features.sort(() => 0.5 - Math.random()).slice(0, 3), // returns 3 random values from the array | ||
| roomPrice: roomPrice || faker.random.numeric(3), | ||
| roomPrice: roomPrice || faker.string.numeric(3), |
There was a problem hiding this comment.
The code here generates a room body with random values. While this is fine for testing, it might be beneficial to have more control over the data being used in tests. Consider adding parameters to this function that allow you to specify the values you want to use for each field. If no value is provided, then a random value can be generated. This would make your tests more flexible and potentially easier to debug.
Also, the faker.string.numeric(3) method is used to generate a numeric string for the roomName and roomPrice fields. However, these fields are likely expected to be numbers, not strings. You should use a method that generates a number instead, such as faker.datatype.number({ min: 100, max: 999 }).
- roomName: roomName || faker.string.numeric(3),
+ roomName: roomName || faker.datatype.number({ min: 100, max: 999 }),
...
- roomPrice: roomPrice || faker.string.numeric(3),
+ roomPrice: roomPrice || faker.datatype.number({ min: 100, max: 999 }),| export const defaultRoom = { | ||
| roomid: 1, | ||
| roomName: "101", | ||
| type: "single", | ||
| accessible: true, | ||
| image: "https://www.mwtestconsultancy.co.uk/img/testim/room2.jpg", | ||
| description: | ||
| "Aenean porttitor mauris sit amet lacinia molestie. In posuere accumsan aliquet. Maecenas sit amet nisl massa. Interdum et malesuada fames ac ante.", | ||
| features: ["TV", "WiFi", "Safe"], | ||
| roomPrice: 100, | ||
| }; |
There was a problem hiding this comment.
This hardcoded defaultRoom object could potentially lead to brittle tests if the default room changes in the future. Consider fetching the default room from the API at the start of your test suite and storing it in a variable. This way, your tests will always use the current default room, even if it changes. If this is not possible due to constraints, please disregard this comment.
let defaultRoom;
beforeAll(async () => {
// Fetch the default room from the API and store it in the defaultRoom variable
defaultRoom = await fetchDefaultRoom();
});
There was a problem hiding this comment.
Files selected (1)
- lib/helpers/createAssertions.ts (1)
Files skipped from review due to trivial changes (1)
- lib/helpers/createAssertions.ts
Review comments generated (0)
- Review: 0
- LGTM: 0
Tips
Chat with CodeRabbit Bot (@coderabbitai)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@coderabbitaiin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignoreanywhere in the PR description to pause further reviews from the bot.
Summary by CodeRabbit
New Features:
createRandomBookingBodyandcreateFutureBookingfunctions inbooking.ts.retriesconfiguration option from0to2inplaywright.config.ts.Tests: