getPageByReference #16

rodikh · 2020-09-18T12:15:18Z

added method to unibit
added basic tests for unibit-functions

- added method to unibit - added basic tests for unibit-functions

- updated tests

rodikh · 2020-09-18T12:20:26Z

tests/unibit-functions.test.js

+ expect(data).toBeNull();
+
+ data = unibit.getData(pageContext, 'data/fake');
+ expect(data).toBeNull();


@smnh be aware that getPage returns undefined when not found, while getData returns null

you can improve it to return null. but validate that it won't break anything else

smnh · 2020-09-18T13:40:27Z

src/unibit.js

@@ -240,6 +240,7 @@ module.exports = class Unibit {
 context.getPage = this.getPage.bind(this, context);
 context.getPages = this.getPages.bind(this, context);
 context.getData = this.getData.bind(this, context);
+ context.getPageByReference = this.getPageByReference.bind(this, context);


I am having second thoughts about the name of this function.
Should it really be getPageByReference? Maybe a better name would be getPageByFilePath because it reflects what this function does.

smnh · 2020-09-18T13:45:47Z

src/unibit.js

+ getPageByReference(context, reference) {
+ // remove extension
+ reference = reference.replace(/\.md$/, '');
+ return _.find(context.site.pages, page => {
+ const pageUrl = _.trim(_.get(page, 'url'), '/');
+ return reference === pageUrl;
+ });
+ }
+


Every page object should have relPath property with a path to the page relative to pagesDir.
Use that instead, because page.url might actually be different from page.relPath if some kind of permalink is used that places the page under a different URL than the page file.

pretty URLs:
/content/about/index.md => /content/about

permalinks:
/content/hello.md => /content/foo

--- title: Hello permalink: /foo ---

smnh · 2020-09-18T13:52:02Z

tests/unibit-functions.test.js

+ it('gets a page by url', () => {
+ let page = unibit.getPage(pageContext, 'about');
+ expect(page).toBe(pageContext.site.pages[0]);
+ });


Don't use huge file just to check correct page selection.
Use data to test only what is needed:
And don't assume indexes in test data but use references

const expectedPage = {url:"...",...smallestDataNeededForTheTest}, const pages = [ {...smallestDataNeededForTheTest}, expectedPage, {...smallestDataNeededForTheTest} ] const page = unibit.getPage({site:{pages:pages, 'about'); expect(page).toBe(expectedPage);

smnh · 2020-09-18T13:53:44Z

tests/unibit-functions.test.js

+ const pageContext = require('./mocks/pageContext.mock.json')
+ it('gets pages from folder', () => {
+ let pages = unibit.getPages(pageContext, 'features/');
+ expect(pages).toStrictEqual([pageContext.site.pages[1], pageContext.site.pages[2], pageContext.site.pages[3]]);
+ });
+ it('returns empty array if not found', () => {
+ let pages = unibit.getPages(pageContext, 'fake/');
+ expect(pages).toStrictEqual([]);
+ });


same here, use smallest dataset for testing, no need to load huge file

smnh · 2020-09-18T13:54:32Z

tests/unibit-functions.test.js

+ expect(data).toBeNull();
+
+ data = unibit.getData(pageContext, 'data/fake');
+ expect(data).toBeNull();


you can improve it to return null. but validate that it won't break anything else

smnh · 2020-09-18T13:55:07Z

tests/mocks/pageContext.mock.json

@@ -0,0 +1,409 @@
+{
+ "site": {


remove this file, use the smallest data set needed to run each test.
this will make it clear what are you testing

- PR fixes - renamed getPageByReference to getPageByFilePath - removed mock file

smnh · 2020-10-01T09:47:21Z

src/unibit.js

@@ -367,6 +368,15 @@ module.exports = class Unibit {
 return _.get(context.site.data, path, null);
 }

+ getPageByFilePath(context, filePath) {
+ // remove extension
+ const urlPath = _.trim(filePath, '.md');


why not _.trimEnd()

smnh · 2020-10-01T09:48:54Z

src/unibit.js

+ return _.find(context.site.pages, page => {
+ const pageUrl = _.trim(_.get(page, 'relPath'), '.md');
+ return urlPath === pageUrl;
+ });


wait, if both have .md, why to trim it?
maybe it is even better to leave extension, so if you have two pages, home.md and home.yml it will work correctly by specifying which extension user want to use

smnh · 2020-10-01T09:50:56Z

tests/unibit-functions.test.js

+ "absPath": "/Users/rodik/Work/Stackbit/project-repos/unibit/sample/unibit-universal/content/about.md",
+ "relPath": "about.md",
+ "absDir": "/Users/rodik/Work/Stackbit/project-repos/unibit/sample/unibit-universal/content",
+ "relDir": "",
+ "url": "about/",
+ "basename": "about.md",
+ "filename": "about"


if absPath (and other props) is not used in the test you can safely remove them from mock data. Only use what is needed by the test. This way test is more strict.
Also, having your personal file system paths in a public repo looks strange

smnh · 2020-10-01T09:56:07Z

tests/unibit-functions.test.js

+ "relPath": "about.md",
+ "absDir": "/Users/rodik/Work/Stackbit/project-repos/unibit/sample/unibit-universal/content",
+ "relDir": "",
+ "url": "about/",


does Unibit generate url with trailing slash?
The mock data must resemble the actual data as close as possible.

I pulled this data directly from the debugger context when running unibit.

smnh · 2020-10-01T09:58:37Z

tests/unibit-functions.test.js

+ it('should not work with file paths', () => {
+ let page = unibit.getPage(pageContext, 'about.md');
+ expect(page).toBeUndefined();
+ });


Although sometimes it is useful to create tests that test failures, in this case, the test should be less concerned with when it fails, but when it needs to work.
Test different ways of invoking the tested function: about/, /about, /about/

smnh · 2020-10-01T10:00:55Z

tests/unibit-functions.test.js

+ it('gets a page by url', () => {
+ let data = unibit.getPageByFilePath(pageContext, 'features/feature1');
+ expect(data).toBe(feature1Page);
+ });


should actually fail,
these functions must be strict, getPage (by URL), getPageByFilePath (by file path)

- PR fixes

getPageByReference

50346cb

- added method to unibit - added basic tests for unibit-functions

rodikh requested a review from smnh September 18, 2020 12:15

getPageByReference

112916a

- updated tests

rodikh commented Sep 18, 2020

View reviewed changes

smnh requested changes Sep 18, 2020

View reviewed changes

getPageByReference

c5d9e26

- PR fixes - renamed getPageByReference to getPageByFilePath - removed mock file

smnh requested changes Oct 1, 2020

View reviewed changes

getPageByReference

95c14e4

- PR fixes

rodikh merged commit 1af72ec into master Oct 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

getPageByReference #16

getPageByReference #16

rodikh commented Sep 18, 2020

rodikh Sep 18, 2020

smnh Sep 18, 2020

smnh Sep 18, 2020

smnh Sep 18, 2020

smnh Sep 18, 2020

smnh Sep 18, 2020

smnh Sep 18, 2020

smnh Sep 18, 2020

smnh Oct 1, 2020

smnh Oct 1, 2020

smnh Oct 1, 2020

smnh Oct 1, 2020

rodikh Oct 1, 2020

smnh Oct 1, 2020

smnh Oct 1, 2020

getPageByReference #16

getPageByReference #16

Conversation

rodikh commented Sep 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment