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

Extract the resolving of a page's path and test it #232

Merged
merged 3 commits into from Apr 9, 2016

Conversation

benstepp
Copy link
Contributor

@benstepp benstepp commented Apr 4, 2016

  • Extracts the resolving of a page's path to it's own file for
    testability.
  • Adds ava as a development dependency to write and run tests.
  • Adds ava related settings to inherit the babel config and run the
    tests with npm test to package.json
  • Refactor the quad-nested if statements in path resolving.

Working toward resolving Issue #154 one step at a time.

Not too sure on code style, so I didn't want to start adding es6 classes when there were none in the project. I opted for a closure with named functions to provide some readability instead of a super-if statement. The named functions also open up a nice readable place to fix user defined paths in frontmatter (#223) and allow for absolute/relative paths defined in frontmatter(#221).

Also not too sure if the gatsby team has any testing preferences on how to organize tests / readable descriptions for tests. I just matched the lib file structure in the test directory. First time using ava (well second. we tried it on an internal project at work and it started to choke around 100+ test files, but the ava team is working on fixing that).

Let me know if you have any feedback/review.

* Extracts the resolving of a page's path to it's own file for
testability.
* Adds `ava` as a development dependency to write and run tests.
* Adds ava related settings to inherit the babel config and run the
tests with npm test to package.json
* Refactor the quad-nested if statements in path resolving.

Working toward resolving Issue gatsbyjs#154
@KyleAMathews
Copy link
Contributor

Wow! This is fantastic. A great refactor + tests! Yeah I don't really like classes generally. The approach you came up with is very readable and should be easy to maintain/extend going forward.

I don't have strong feelings about tests structure so what you've setup sounds great.

@gatsbyjs/gatsby-core-maintainers any thoughts?

@thangngoc89
Copy link
Contributor

+1 for using AVA as test runner. Although be aware that AVA will probably break on old node (<5) if there are too many files

* Split up all the spaghetti in glob-pages up so that it can be unit
tested.
* Renamed `pathresolver` to `urlResolver`
* Added tests to verify the structure of the data returned by the new
pathResolver.
* updated the urlResolver's tests with the new function name
@benstepp
Copy link
Contributor Author

benstepp commented Apr 6, 2016

I had some time tonight and split up glob pages to the point where I think it can be unit tested effectively. Do you guys see any problems with the way that I split it up? Would you have split it up differently?

Renamed the pathResolver from the first commit to urlResolver now that I have a clearer idea of what was happening in glob-pages. 100+ line functions really take a lot of cognitive load to fully understand and follow effectively. I can still build the blog-starter, so I don't think that I've broken anything. It's always hard to tell though without tests.

On a side note I just noticed while looking for some documentation that ava is changing the ok/notok to falsey/truthy in the very near future, so some minor refactoring will be required.

@KyleAMathews
Copy link
Contributor

@benstepp really awesome stuff! I didn't have time to get it a full review but things looked good on my first path through. I'll give this a through review this Saturday (along with the other open PRs). And 100% agree glob-pages needed broken up.

@scottnonnenberg
Copy link
Contributor

I love that we're bringing tests into the project! I have felt the pain of making changes in gatsby without them. However, unit tests are always a bit scary by themselves, so we now have a false sense of security about the whole project if npm test succeeds.

Would you consider adding a basic overall test scenario? Maybe just a couple files, run gatsby, validate that it runs successfully, puts files on disk?

@KyleAMathews
Copy link
Contributor

@scottnonnenberg I totally agree e2e tests are needed. I have a number of ideas toward that end in #154. If @benstepp (or you) want to tackle that hallelujah but I think @benstepp meant this PR to be an incremental step along the way to full test coverage :-)

@benstepp
Copy link
Contributor Author

benstepp commented Apr 6, 2016

#226 would be a good place to start. Adding them as submodules / test-fixtures could integrate them into the test suite. This does add a lot of developer overhead though, as the dependency count is pretty big, and we would have to keep them up to date.

Not sure if you would want to do a full integration test with actually calling the cli in shell, or abstract the cli from gatsby so that all it does is call a function to build and we would test the build function in the suite.

Also I don't think ava has test-tagging yet. With RSpec (ruby land, super strong) you could tag a test as slow and then skip running them, but I'm sure we could come up with something. Maybe a naming convention with test files and then two package.json scripts to target specifically named files and run all tests or the non-slow tests.

And yes, the PR is meant as an incremental step. Writing the first tests for a project are always hard, and you would usually test from the outside-in (integration into unit tests), but I felt like that would require too much reorganization to pull off quickly.

@KyleAMathews
Copy link
Contributor

@KyleAMathews KyleAMathews merged commit 3434e49 into gatsbyjs:master Apr 9, 2016
@KyleAMathews
Copy link
Contributor

@benstepp fantastic work! This is a really great start to getting Gatsby's testing story going! Thanks for all your contributions so far. I added you as a contributor to the repo btw :-)

@KyleAMathews
Copy link
Contributor

I did need to make one minor fix to your refactoring as metadata from pages weren't being used in finding the path or returned correctly. 0ce3be5

@KyleAMathews
Copy link
Contributor

I also added a eslint plugin for linting Ava which seems pretty cool 5ab7a0c https://github.com/sindresorhus/eslint-plugin-ava

@benstepp benstepp deleted the bs-testing branch April 16, 2016 22:25
@jlengstorf
Copy link
Contributor

Hiya @benstepp! 👋

This is definitely late, but on behalf of the entire Gatsby community, I wanted to say thank you for being here.

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

  1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. (We’ve got t-shirts and hats, plus some socks that are really razzing our berries right now.)
  2. If you’re not already part of it, we just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. You’ll receive an email shortly asking you to confirm. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If you have questions, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again! 💪💜

This was referenced Jun 6, 2021
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.

None yet

5 participants