Tests

[martink-rsa]

Writing tests for the app, currently in progress.

[martink-rsa]

I've written the following tests, not limited to:

1. Header/Footer:
   1.1 Header and footer loads
2. HomePage:
   1.1 Elements render
   1.2 Route to /search/ is working
3. SearchPage:
4. Loading spinner shows and disappears
5. Default subreddit is loaded
6. User-changed subreddit is loaded
7. Heatmap is loaded with all buttons, values are correct and correct colours are used.
8. Posts table is loaded and rows are created

[jkettmann]

All in all excellent work. Very detailed tests covering every detail afaik. You can simplify some tests by making use of the async findBy* instead of getBy* queries.

[jkettmann]

I think it would be enough to use getByText here instead of getByTestId here and below. Here is a document of priorities given to the single queries. It's recommended to try testing like a real user. Test IDs should be the last resort

[martink-rsa]

I've been thinking of doing that, but I'm sitting with a mental dilemma.

I would have to test for the strings in the links in the header, but then these tests will fail if someone ever changes the contents of the links. I thought that tests shouldn't be written this way, something along the lines of "tests shouldn't break if someone changes the content of the site".

[martink-rsa]

I haven't been able to make this change. I will handle the others and come back to it.

[jkettmann]

That's a good point. At the same time the wordings don't change so often typically. And if somebody changed the text inside an important link or button the tests would catch it.

Honestly, I think there are advantages to both approaches. I'd just go for the recommended one because of the reasons above and because you don't need so many test IDs in your components

[jkettmann]

It's a good idea to extract the path into a config, but the name is not so descriptive. Maybe call it SEARCH_PATH. It might also be good to create a function that builds the path since the /${DEFAULT_PATH}/${DEFAULT_SUBREDDIT} is spread over multiple components now. Something like getSearchPath that could look like this

function getSearchPath(subreddit = DEFAULT_SUBREDDIT) {
  return `/${SEARCH_PATH}/${DEFAULT_SUBREDDIT}`
}

[jkettmann]

Here the testid is not needed as well

[jkettmann]

The testing stuff should go to dev dependencies to not bloat the production build

[martink-rsa]

"@testing-library/jest-dom": "^5.1.1", will throw a ESLint error, but the rest were moved.

[jkettmann]

Is TZ=UTC used to have a constant timezone? Is there documentation for that in the jest docs or so? Just curious

[martink-rsa]

I found it while Googling for a solution for getting the timezone to always be the same. According to others, it works without problems, however, some say it might not work on Windows. I don't have a dev setup for Windows so I can't verify this. I wasn't able to source the docs for it, but I'm guessing it's part of Webpack.

nodejs/node#4230

I will probably switch to a package in the future to emulate a timezone. Still thinking of a way to approach it because if I move to Moment, then I may as well rewrite my dates to use Moment instead of my own functions. In that case, it will be a refactor iteration.

[jkettmann]

Great, if it's working then it's fine. I experienced in a lot of companies that they don't care so much about devs on Windows since almost everyone is using Mac anyway with a few outliers using Linux.

[martink-rsa]

I've tried to get rid of IDs (getByIds etc.) such as trying to find a header and footer tag using role but wasn't able but couldn't get it to work. Looking into it I think it's meant more for Aria roles.

jest-axe was used to do a wide accessibility check which seems to be great to find missing parameters such as alt tags, but it's not a complete solution as I noticed that there are still contrast levels that don't pass but aren't found by the test.

[jkettmann]

Don't worry about the ids, just keep it in my for the next time. I'd still suggest you refactor the things I mentioned here.

[martink-rsa]

Don't worry about the ids, just keep it in my for the next time. I'd still suggest you refactor the things I mentioned here.

Not sure how I missed this, thank you. I will be changing it.

[martink-rsa]

I've refactored to remove a lot of the redundancy that I had e.g. assigning a variable to the element as well as checking if it was in the document. Now I only assign the variable.

I don't know if I've gone too concise though as I've also refactored some parts to not include a variable and use the query within the check e.g.

expect(getByTestId('heatmap')).toBeInTheDocument();

I feel there is not much further I can reduce but this could just be a limitation of my skills.

[jkettmann]

Looks great. I added a comment. Change your code if you like. If not feel free to merge ;-)

[jkettmann]

You could use .map here

const posts = currentTable.map(({ author, title }) => ({
  ...
)};

[martink-rsa]

Trying to map it but I'm not sure if it's possible with what I'm doing. I'm creating a 1d array from all of the table cells so I can test each one easily. The reason for doing this was because it became to complex to loop and test the data while it was in its complex object form. This is why there are multiple push statements in each loop:

    posts.push(author);
    posts.push(title);
    posts.push(created);
    posts.push(score);
    posts.push(num_comments);

currentTable's values are rows in the table which are 5 different key/value pairs. map is only going to give me one opportunity/return to deal with all of the key/values for that particular row so I would only be able to return an entire row instead of returning each value so that the data is in a flattened 1d array.

The only thing I can think of at the moment is to keep it as a row (2d array) from the .map array and then use .flat to flatten it before it's returned.

[martink-rsa]

I think I might be able to do it with 2 maps, the 2nd looping through keys. Attempting this at the moment and then will do a comparison at the end.

[jkettmann]

You're right, map won't work. I didn't read the code properly. You can use reduce though.

const posts = currentTable.map((tmpPosts, { author, title }) => ({
  return tmpPosts.concat(author, title, created, score, num_comments);
)}, []);

Didn't test the concat so be careful :-)

[jkettmann]

Btw the name posts is a bit confusing here. That's why I expected it to be an array of post objects.

[martink-rsa]

Btw the name posts is a bit confusing here. That's why I expected it to be an array of post objects.

You're right. I changed it as soon as you did the review as I immediately saw it was the cause of confusion. It was originally for each post, but the code evolved quite a bit and naming got lost along the way. Thanks.

[martink-rsa]

I don't think it's going to be possible to do it elegantly with array methods, namely for the following reason:

1. I can't map through all of the keys as I have keys in my object, such as the Reddit post URL, but I'm not testing for the Reddit URL in the test.

2. The test itself loops through the array and checks for the item's text to be in the document. Since the data is a 1d array with no keys/values, I can't separate out content, such as the URL, so that it's not being tested for. I can't switch back to passing an object to the test as that creates massive problems when it comes to testing and trying to loop through each of the objects.

What I've got is, but the test will break for reasons above. I have to flatten the solution below otherwise I have nested arrays:

function getPosts(data, day, hour) {
  const currentTable = data[day][hour];
  const cells = currentTable.map((row) => {
    return Object.keys(row).map(key => {
      if (key === 'created') {
        return displayHHMM(row[key]);
      }
      return row[key].toString();
    });
  }).flat();
  return cells;
}

I hate giving up on a coding problem, but I'm starting to think that the way I did initially is going to be one of the more elegant solutions to handle it.

I await your input. I don't mind going at it for as long as I need to get it right, just not sure if I'm going to find something that works better.

[jkettmann]

Sorry, the code in the comment was wrong. I wanted to use reduce instead of map.

const posts = currentTable.reduce((tmpPosts, row) => ({
  // Convert date obj to "hh:mm" with am/pm suffixed
  const created = displayHHMM(row.created);
  // Can't test numeric values, need to convert to string
  const num_comments = row.num_comments.toString();
  const score = row.score.toString();
  return tmpPosts.concat(row.author, row.title, created, score, num_comments);
)}, []);

[jkettmann]

This will give you a 1D array with exactly like the one in your code if I'm right

[martink-rsa]

Amazing!! I tried it the first time (I knew to switch map with reduce) but couldn't figure out the syntax error it had.

Here it is in its glory with the syntax error fixed and variables renamed. Thank you so much!

function getCellData(data, day, hour) {
  const currentTable = data[day][hour];
  const cells = currentTable.reduce((tmpCells, row) => {
    // Convert date obj to "hh:mm" with am/pm suffixed
    const created = displayHHMM(row.created);
    // Can't test numeric values, need to convert to string
    const num_comments = row.num_comments.toString();
    const score = row.score.toString();
    return tmpCells.concat(row.author, row.title, created, score, num_comments);
  }, []);
  return cells;
}

[jkettmann]

Since this assertion is repeated often: a nice way to prevent this kind of repetition is to create a function inside setup.

const setup = () => {
  const queries = render(...)

  const waitForData = async() => {
    await waitForElementToBeRemoved(getByTestId('loading-spinner'));
  }
	
  return {
    ...queries,
    waitForData,
  }
}

and then use it here like

await waitForData();

This way it's a bit more descriptive what the assertion is doing.

You can also create functions that are more complex like searchNewSubreddit where you enter a new subreddit and submit the form or so. Just as an example, not saying that you should do that here.

Here is a nice blog post AHA testing

[jkettmann]

👍