Skip to content

Adding initial unit tests#2

Merged
thescientist13 merged 7 commits into
masterfrom
task/unit-testing
Mar 28, 2019
Merged

Adding initial unit tests#2
thescientist13 merged 7 commits into
masterfrom
task/unit-testing

Conversation

@hutchgrant

Copy link
Copy Markdown
Member

resolves #1

Adds unit tests for production file creation. Also tests basic rendering.

Problem: it doesn't successfully exit because of the local-web-server running for the render tests. If you comment out the render tests it ends successfully.

@thescientist13 thescientist13 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is looking pretty good, just left some feedback that double solved the exit issue. You're on the right track!

Comment thread test/cli.spec.js Outdated

describe('when rendered', () => {
let page;
const localWebServer = new LocalWebServer();

@thescientist13 thescientist13 Mar 26, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So I have some good news and some good news. 😄

The reason isn't exiting is a simple one I believe. You aren't calling close, e.g.

server.close()

That said, I wouldn't expect to actually need to start the local server as part of the testing. By running index.js, we already get generated build output (which is what we want).

Instead, I think what we would want to test is that our "input" is in our output.

So the it block might be better off reading the original contents of hello.md and making sure that text is being asserted against the already generated output of running index.js.

In a testing , this is commonly referred to as using fixtures.

This comment was marked as outdated.

Comment thread test/cli.spec.js Outdated
publicDir: path.join(__dirname, '..', './public')
};

before(async () => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe it's just me, but this and after "look" funny to me outside of a describe block. What do you think of nesting any before or after within a describe?

I think it helps in aiding the structure / organization of the tests and any particular setup / teardown work needed on a per describe basis, including nested ones.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't know, made sense that before testing x, run the script, after testing x, remove everything. I've updated it to be your way though.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In concept, it's basically just creating mock files that can be used in tests to mock API requests or databases. (kind of like Jest snapshot testing)

For this though, all I'm suggesting is literally use the source files in the tests to be the expected part of the assertion. We should be testing the index.js script created the site correctly, so we shouldn't be running the server in our tests, just testing expected vs actual.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Right but mock tests won't allow us to check for problems as a result of webpack failures during build. For example, when I added the front-matter import var for css and the index.html didn't compile correctly. Even though it existed, and was the correct path, its contents were incorrect because it was a 404 page that then became serialized and outputted to file. Hence the need to actually render the page and test it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Right, but I saw we were testing for the contents inside the page, so if

This is the home page built by Greenwood. Make your own pages in src/pages/index.js!

Is not found in the output file, then wouldn't the test fail?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For example, when I added the front-matter import var for css and the index.html didn't compile correctly.

Also, this isn't a supported feature yet, all we should be doing is trying to test the default build as robustly as we can, and then when features for CSS and other assets get defined and issue made, we would want to add unit testing for that new feature at that time.

@hutchgrant hutchgrant Mar 27, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

no it's not just that feature, thats just an example how it could fail. What I'm testing for is if it serialized correctly, which a mock test would not be able to test.

edit: perhaps I should change the test description to "serialize correctly"

@thescientist13 thescientist13 Mar 27, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe we can talk on Slack, but I think a more robust way of testing is to take the actual output, and test that directly (against our own project's source files).

Additionally, it would negate the need for our tests to require starting up a webserver for each one.

Some ways to do this that come to mind are:

  1. Read the output file to string, and and do a string search of the expected content that should be there (little hacky)
  2. Read the output file into a DOM library (like JSDOM?) and assert tests that way

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

option 2 is preferred i think

@thescientist13

thescientist13 commented Mar 26, 2019

Copy link
Copy Markdown
Member

also the project is currently using Yarn. 😼

Comment thread test/cli.spec.js
it('should display the hello world text', async () => {
let paragraph = dom.window.document.querySelector('p.wc-md-hello').textContent;

expect(paragraph).to.equal('This is an example page built by Greenwood. Make your own in src/pages!');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

how hard would it be to actually get this asserted text from the actual source files of the project itself, like using fs?

@hutchgrant hutchgrant Mar 28, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Would have to parse the markdown file for it, I don't know.

@thescientist13 thescientist13 Mar 28, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ok, I wouldn't expect it to be too hard (but the value in doing so is high), so will make an issue for it, and maybe some others for the next project to add some unit testing to our code in /lib.

@hutchgrant hutchgrant Mar 28, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

markdown-it can parse it but not like jsdom, it breaks it down into an array of objects and unfortunately I can't get it to filter out italics.

  -This is an example page built by Greenwood.  Make your own in src/pages!
  +This is an example page built by Greenwood.  Make your own in _src/pages_!

if we're expecting a certain tag, we can assume the the next spot in array will have the content for the tag. e.g. we find an h3, we know the next pos in array will have our 'Hello World' heading content. That works.

const markdown = require('markdown-it');
const md = new markdown();
const fs = require('fs-extra');
const path = require('path');

let dom, mark, it = 0;
const mdLine = [];
const file = await fs.readFile(path.resolve(__dirname, '..','./packages/cli/templates/hello.md'), 'utf-8');

mark = md.parse(file, {});
mark.filter((ln, i) => {
    if(ln.tag === "h3" || ln.tag === "p"){
        it++
        mdLine.push(i+it);
     }
 });
            
mdLine.map(item => {
  console.log(mark[item].content)
})

@thescientist13 thescientist13 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good stuff, excited to get this in so soon!

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.

As a developer, I would like unit tests

2 participants