Adding system test

[reficul31]

This is a top level test that I thought was extremely necessary for the system. I would rather have the file moved to its own file, but I would be cool with anything @Treora and @sanjo decides.

[Treora]

Some high level test would indeed be nice. Would it make sense to create a folder test for these, while keeping the .test.js files for unit tests? Or is that just confusing? Would be nice to then also put the html string into an actual html file (possibly next to the html file with the desired output; or do you think jest's snapshot mechanism is more practical for this?).

It currently looks like the snapshot output is not processed at all, it still contains scripts etcetera; am I understanding something incorrectly, or might the mocks have to be restored after the first test?

[reficul31]

Having the system tests in their own directory would make a lot of sense and allow us to run them in parallel also. This is the approach first suggested by @sanjo as well.
Regarding the snapshot output, I hope I understood your query correctly. If you see at line , toMatchSnapshot is used. This is used to check every part of the html string returned back from the freeze dry module. It really makes it much more simpler to have this, instead of checking to see if every script, every url etc was fixed. toMatchSnapshot basically just checks the whole string and is very easy to update would the test be likely to change. Just run jest -u and the snapshot will update with whatever changes were introduced, essentially rewriting the snap file.
Ps. Please do tell if the test directory approach suits you, I would like to change the PR in that case.

[Treora]

I did still mean to compare the output to the whole snapshot string, I just wondered whether that string could be read from a file; so you'd have e.g. test/test.js, test/example.html, test/example_output.html. This would be neat, but the downside is that then we would not have the convenience of running jest -u to update that output file.

Regarding the output not being processed, have a better look at src/__snapshots__/index.test.js.snap. It does not look freeze-dried.

[reficul31]

I thought I ran it through every test case. Could you be more specific?
PS. I changed the directory structure according to the above comment.

[Treora]

Ok, the snapshot looks processed now as it should; I assume the problem was that when it was in the same file as the unit test, the unit test mocked all processing functions and did not restore them, so they were not executed during this test either.

By the way, it would be helpful if you would not amend your commits during back-and-forths like these. That would help us to see clearly what changed since last review. The commits could then still be squashed to one commit before merging the PR.

A problem with this test (as also commented inline) is now that the fetch mock always returns the image, also for the stylesheet. It is unfortunate we cannot give jest-fetch-mock a map of url-response pairs; to get the right responses for each URL we would have to assume the order in which they are fetched, which would be too fragile. I am wondering if we could somehow use the real fetch, or find/make a replacement that reads local files, so we can just put the resources (image and stylesheet) as files in the same folder and fetch will just get them.

Also, I would make the html file an actual html file, not a script that exports a string. Instead of importing it, you may then have to use something like fs.readFile to get the string.

[Treora]

We need something better here, not every request should return the image.

[Treora]

This looks rather wrong; the fetch for the stylesheet returns the image..

[reficul31]

It is unfortunate we cannot give jest-fetch-mock a map of url-response pairs; to get the right responses for each URL we would have to assume the order in which they are fetched, which would be too fragile.

How about having a look into fetch-mock . The other option we have is to write a mock of the fetch api that can get the files from the local file system. Something along the line of this, but modified for the fetch api.

By the way, it would be helpful if you would not amend your commits during back-and-forths like these. That would help us to see clearly what changed since last review. The commits could then still be squashed to one commit before merging the PR.

Yeah, sorry about that. Force of habit, feel its much cleaner to have less number of commits. But yeah I get your point. I'll try and push changes in consecutive commits.

[reficul31]

@Treora
Sorry for being absent for these few days. I have added the example.html file and am now reading the text through fs.readFileSync. I have also added the test for idempotency. As currently the feature is under development, the test will be skipped. It will come in handy while releasing the feature, just remove the test.skip with test and the test should run. I hope I have covered all the functions of the module.
I have proposed a solution for the fetch mock. Please have a look and tell me if it seems like a viable option.

[Treora]

I added a test similar to (and inspired by) this work in the rewrite (commit 6821123). Closing this.