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

Add some unit test #11

Merged
merged 19 commits into from
Feb 27, 2018
Merged

Add some unit test #11

merged 19 commits into from
Feb 27, 2018

Conversation

jihchi
Copy link
Collaborator

@jihchi jihchi commented Feb 24, 2018

Screenshot

screenshot_2018-02-25_19-59-39

@jihchi
Copy link
Collaborator Author

jihchi commented Feb 25, 2018

@zploskey done.

@zploskey
Copy link
Owner

Overall this looks great. There are a couple of minor issues that probably would have been caught by CI. That's something I plan to set up.

Copy link
Owner

@zploskey zploskey left a comment

Choose a reason for hiding this comment

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

This is great! Just a couple of minor adjustments needed.

package.json Outdated
@@ -6,7 +6,8 @@
"scripts": {
"build": "bsb -make-world",
"start": "bsb -make-world -w",
"clean": "bsb -clean-world"
"clean": "bsb -clean-world",
"test": "jest --watch"
Copy link
Owner

Choose a reason for hiding this comment

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

Using jest --watch does not seem to work for me. We could just use jest, which is probably more what we'll usually want anyways, and gives us a target to run in CI. It gives the following error:

Error: watch /home/zach/src/bs-puppeteer ENOSPC
    at _errnoException (util.js:1022:11)
    at FSWatcher.start (fs.js:1374:19)
    at Object.fs.watch (fs.js:1400:11)
    at NodeWatcher.watchdir (/home/zach/src/bs-puppeteer/node_modules/sane/src/node_watcher.js:175:20)
    at new NodeWatcher (/home/zach/src/bs-puppeteer/node_modules/sane/src/node_watcher.js:45:8)
    at createWatcher (/home/zach/src/bs-puppeteer/node_modules/jest-haste-map/build/index.js:572:23)
    at Array.map (<anonymous>)
    at HasteMap._watch (/home/zach/src/bs-puppeteer/node_modules/jest-haste-map/build/index.js:683:44)
    at _buildPromise._buildFileMap.then.then.hasteMap (/home/zach/src/bs-puppeteer/node_modules/jest-haste-map/build/index.js:280:21)
    at <anonymous>

Copy link
Collaborator Author

@jihchi jihchi Feb 27, 2018

Choose a reason for hiding this comment

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

Error: watch /home/zach/src/bs-puppeteer ENOSPC
It seems like you're running out of space.
I found this issue from Jest, Please give it a try:

echo fs.inotify.max_user_watches=524288 | sudo tee -a /etc/sysctl.conf && sudo sysctl -p

However, I've changed to jest for test and added test:watch for watch mode: . (ref.: a3fad57)

src/FrameBase.re Outdated
@@ -47,3 +47,7 @@ external waitForXPath :
(~xpath: string, ~options: selectorOptions=?, unit) =>
Js.Promise.t(ElementHandle.t) =
"";

[@bs.send]
external selectOneEval : (t, string, unit => unit) => Js.Promise.t('a) =
Copy link
Owner

@zploskey zploskey Feb 25, 2018

Choose a reason for hiding this comment

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

So far we've been using the convention of having the object last so that things pipe nicely with |>. If you change this to [@bs.send.pipe : t], remove t from the args, and adjust the test so it runs that would make it consistent with the rest of the bindings. I've got more work on the various eval bindings coming.

src/Page.re Outdated
@@ -95,8 +99,8 @@ external setExtraHTTPHeaders : (~headers: Js.Dict.t(string), unit) => Js.Promise

type typeOptions = {. "delay": float};

[@bs.send.pipe : t]
Copy link
Owner

Choose a reason for hiding this comment

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

Let's keep it using bs.send.pipe for now, at least until there's an object first pipe syntax available. Same for setContent, and Page.type_, though it doesn't really matter for content.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in commit 1c1183a, 3d64908 and c8c07aa.


describe("Puppeteer", () =>
test("executablePath", () =>
Puppeteer.executablePath() |> expect |> toContainString("/Chromium")
Copy link
Owner

Choose a reason for hiding this comment

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

This fails for me with the following:

  ● Puppeteer › executablePath

    expect(received).toEqual(expected)
    
    Expected value to equal:
      StringContaining "/Chromium"
    Received:
      "/home/zach/src/bs-puppeteer/node_modules/puppeteer/.local-chromium/linux-536395/chrome-linux/chrome"
      
      at affirm (node_modules/@glennsl/bs-jest/lib/js/src/jest.js:145:40)
      at Object.<anonymous> (node_modules/@glennsl/bs-jest/lib/js/src/jest.js:266:11)

@jihchi
Copy link
Collaborator Author

jihchi commented Feb 27, 2018

@zploskey All fixed.

@zploskey zploskey merged commit a3a9551 into zploskey:master Feb 27, 2018
@jihchi jihchi deleted the add_unit_test branch February 27, 2018 23:14
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.

2 participants