Skip to content
This repository was archived by the owner on Jun 1, 2023. It is now read-only.

Conversation

@ethanimooney
Copy link
Contributor

TL;DR: Added the first of the JS frontend tests, focusing on paginationScript.js
Note: I may have done these terribly if I'm totally honest - please let me know what I can improve and change :)

,gitignore - Added ignore for node_modules/ , as I had to install Node.js to utilize Jest

package.json package-lock.json - two files needed for Node and Jest

paginationScript.test.js - the actual testing file. Spoke with shaquilleh@ and decided to test the working of the algorithm, rather than the DOM elements themselves. This file contains tests of the main functions, using arrays and objects instead of DOM elements.

Another Note: I also realize these may be lacking, but I wanted to make sure that I get review on the initial ones to make sure I am on the right track.

@ethanimooney ethanimooney added enhancement New feature or request tests labels Jul 24, 2020
@ethanimooney ethanimooney added this to the Final Capstone Project milestone Jul 24, 2020
@ethanimooney ethanimooney self-assigned this Jul 24, 2020
@ethanimooney ethanimooney linked an issue Jul 24, 2020 that may be closed by this pull request
let testResultsContent = [];
let testResultsCardsList = [];

for (let i = 0; i < 6; i++) {

Choose a reason for hiding this comment

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

Why 6?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm just filling the testResultsCardList with 6 cards, enough to test what I need it to.

Comment on lines 71 to 74
test('If the moveNext function works as expected', () => {

// Move next (what is being tested)
testDisplayCards(TOTAL_CARDS_TO_DISPLAY);

Choose a reason for hiding this comment

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

Let's highlight what the list augment should be here, as opposed to using TOTAL_CARDS_TO_DISPLAY.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha

Comment on lines 81 to 84
test(' If the navigatePrevious works as expected', () => {

// Move Backwards (what is being tested)
testDisplayCards((-TOTAL_CARDS_TO_DISPLAY));

Choose a reason for hiding this comment

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

Same comment as above.

testResultsCardsList.push(testCardObject);
}

function testDisplayCards(listAugment){

Choose a reason for hiding this comment

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

Rather than rewriting this logic here, can you reference the displayCards method in pagination.js?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would, but the changing from DOM elements to an array of objects wouldn't lend itself to this, so I had to copy and update it for arrays (simple things like appending to an array, etc, no big changes)

@ethanimooney
Copy link
Contributor Author

These new tests should work but they will not run on my computer as Jest seems to have cached the old tests, and is trying to run those instead. Need to sync with shaquilleh@ tomorrow to figure this out.

@ethanimooney ethanimooney requested review from nightrunner500 and removed request for nightrunner500 August 6, 2020 21:56
Comment on lines 20 to 36
test('It calculateAugment performs as expected given certain inputs', () => {
//Beginning of list
currentFirstCardIndex = 0;
expect(calculateListAugment(-TOTAL_CARDS_TO_DISPLAY)).toEqual(0);

//End of list
currentFirstCardIndex = MAX_LIST_VIEW_NUMER - TOTAL_CARDS_TO_DISPLAY;
expect(calculateListAugment(TOTAL_CARDS_TO_DISPLAY)).toEqual(0);

//Some random acceptable location in list, next
currentFirstCardIndex = 3;
expect(calculateListAugment(TOTAL_CARDS_TO_DISPLAY)).toEqual(3);

//Some random acceptable location in list, previous
currentFirstCardIndex = 3;
expect(calculateListAugment(-TOTAL_CARDS_TO_DISPLAY)).toEqual(0);
});

Choose a reason for hiding this comment

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

Each of these expects should be a different test. i.e. testMoveNext(), testMovePrevious, testUserAtEndofList etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

const TOTAL_CARDS_TO_DISPLAY = 3;
const MAX_LIST_VIEW_NUMER = 15;

test('It calculateListAugment performs as expected when at beginning of list', () => {

Choose a reason for hiding this comment

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

Should this say "If calculateListAugment ..."?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, yes it should ... it really is a Friday isn't it

const MAX_LIST_VIEW_NUMER = 15;

test('It calculateListAugment performs as expected when at beginning of list', () => {
currentFirstCardIndex = 0;

Choose a reason for hiding this comment

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

Do you need this variable? I don't see it being used anywhere. Perhaps you intended for calculateListAugment to receive it as a parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's being used in the second test, line 26

Copy link
Contributor Author

Choose a reason for hiding this comment

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

...Mispelled oops

@ethanimooney
Copy link
Contributor Author

As mentioned in the issue for this, the tests are effectively done. I wasn't able to actually run them due to issues with something caching old tests and not running the new ones, which is why this PR hasn't been approved.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

enhancement New feature or request tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Find Solid JavaScript Testing

2 participants