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

Chitter frontend #126

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Chitter frontend #126

wants to merge 6 commits into from

Conversation

ZacMossHK
Copy link

No description provided.

Copy link

@stephenfletchtek stephenfletchtek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall elegant but complex! Easy to read and decent structure

Comment on lines +14 to +18
testPeepsArray = require("./testPeepsArray");
testUserSessionResponse = require("./testUserSessionResponse");
testPostPeepResponse = require("./testPostPeepResponse");
testDeletePeepResponse = require("./testDeletePeepResponse");
fetch.resetMocks();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall the tests are logical and beautifully written. Main question is:

Would the tests have achieved the same objectives with 'simple' data rather than the 'real' examples you've used in each of the files?

You could have all of your test data in one json file (test-data.json for example?)

I like that you've used the example data - but if the tests have the same integrity with simplified data then it would be best to simplify.

fetch("https://chitter-backend-api-v2.herokuapp.com/peeps", {
method: "POST",
headers: {
Authorization: `Token token=${sessionKey}`,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting difference here.

Will also work as 'Token token=' + sessionKey

I don't know which is best.

Comment on lines +24 to +26
resetUserId() {
this.userId = null;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method has not been tested in chitterModel.test.js

Comment on lines +36 to +38
resetSessionKey() {
this.sessionKey = null;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method has not been tested in chitterModel.test.js

Comment on lines +52 to +74
expect(peepDivEls[0].querySelector(".peep-body").textContent).toBe(
"First peep"
);
expect(peepDivEls[0].querySelector(".peep-user-handle").textContent).toBe(
"jony144"
);
expect(
peepDivEls[0].querySelector(".peep-datetime-created").textContent
).toBe("12:33 Sat Aug 20 2022");
expect(peepDivEls[0].querySelector(".peep-likes-count").textContent).toBe(
"1 like"
);
expect(peepDivEls[1].querySelector(".peep-body").textContent).toBe(
"i'm tiz"
);
expect(peepDivEls[1].querySelector(".peep-user-handle").textContent).toBe(
"tiz"
);
expect(
peepDivEls[1].querySelector(".peep-datetime-created").textContent
).toBe("13:04 Sun Aug 07 2022");
expect(peepDivEls[1].querySelector(".peep-likes-count").textContent).toBe(
"0 likes"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test is confirming you fetched two test peeps. You could get away with testing all elements on the first peep and then just one element to confirm the second peep is there. This simplifies the test without a big compromise in integrity.

Comment on lines +180 to +190
expect(newPeepEls[0].querySelector(".peep-body").textContent).toBe(
"my first peep :)"
);
expect(newPeepEls[0].querySelector(".peep-user-handle").textContent).toBe(
"kay"
);
expect(
newPeepEls[0].querySelector(".peep-datetime-created").textContent
).toBe("14:21 Sat Jun 23 2018");
expect(newPeepEls[0].querySelector(".peep-likes-count").textContent).toBe(
"0 likes"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need to test all elements if peep has been created

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants