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 bindings for Coverage #44

Merged
merged 9 commits into from Apr 10, 2018

Conversation

Projects
None yet
2 participants
@jihchi
Copy link
Collaborator

jihchi commented Apr 6, 2018

Add bindings:

Page
    ✓ coverage
Coverage
    startJSCoverage, stopJSCoverage
      ✓ report.ranges[0] (1ms)
      ✓ report.ranges[1]
      ✓ report.text (1ms)
      ✓ report.url
    startCSSCoverage, stopCSSCoverage
      ✓ report.ranges[0]
      ✓ report.text (1ms)
      ✓ report.url

Fix #38

Archie Lee added some commits Apr 6, 2018

@jihchi jihchi requested a review from zploskey Apr 6, 2018

Archie Lee
@zploskey
Copy link
Owner

zploskey left a comment

Looks pretty good. Just a few things to address.

@@ -673,7 +673,7 @@ describe("Coverage", () => {
|> Coverage.startJSCoverage(_, Coverage.makeJSCoverageOptions())
|> then_(() => page |> Page.goto("file://" ++ testPagePath, ()))
|> then_(_res => coverage |> Coverage.stopJSCoverage)
|> then_(report => report |> expect |> toMatchSnapshot |> resolve);
|> then_(report => report |> expect |> toHaveLength(1) |> resolve);

This comment has been minimized.

@zploskey

zploskey Apr 6, 2018

Owner

Having snapshots working would be great. Do you have any idea what exactly the underlying issue is here? The jest docs talk about this here: https://facebook.github.io/jest/docs/en/snapshot-testing.html#snapshots-are-not-written-automatically-on-continuous-integration-systems-ci

This comment has been minimized.

@jihchi

jihchi Apr 7, 2018

Author Collaborator

I got error from: https://travis-ci.org/bs-puppeteer/bs-puppeteer/builds/362974031#L1805

  ● Coverage › startJSCoverage, stopJSCoverage
    expect(value).toMatchSnapshot()
    
    New snapshot was not written. The update flag must be explicitly passed to write a new snapshot.
    
    This is likely because this test is run in a continuous integration (CI) environment in which snapshots are not written by default.
    
    Received value 
    Array [
      Object {
        "ranges": Array [
          Object {
            "end": 19,
            "start": 0,
          },
          Object {
            "end": 63,
            "start": 37,
          },
        ],
        "text": "
    		function foo() {function bar() { } console.log(1); } foo(); ",
        "url": "file:///home/travis/build/bs-puppeteer/bs-puppeteer/__tests__/fixtures/testPage.html",
      },
    ]
      
      
      at affirm (node_modules/@glennsl/bs-jest/lib/js/src/jest.js:184:35)
      at node_modules/@glennsl/bs-jest/lib/js/src/jest.js:269:48

I think the major issue here is url in snapshot, so I just simply change to check the length.

This comment has been minimized.

@zploskey

zploskey Apr 7, 2018

Owner

Ah I see. Well we can at least add a couple of the same checks the snapshot would give us without using snapshots. We can check that the url contains fixtures/testPage.html, and we can check that the ranges are right.

This comment has been minimized.

@jihchi

jihchi Apr 8, 2018

Author Collaborator

Okay. Fixed in commit 583a700

.
"start": float,
"end": float,
};

This comment has been minimized.

@zploskey

zploskey Apr 6, 2018

Owner

Start and end should be integers, since they represent character offsets in the CSS or JS.

This comment has been minimized.

@jihchi

jihchi Apr 7, 2018

Author Collaborator

Fixed in commit 844842b


[@bs.send]
external startCSSCoverage : (t, cssCoverageOptions) => Js.Promise.t(unit) =
"";

This comment has been minimized.

@zploskey

zploskey Apr 6, 2018

Owner

The options parameter should be optional. Likewise for startJSCoverage.

This comment has been minimized.

@jihchi

jihchi Apr 7, 2018

Author Collaborator

Fixed in commit d161884

[@bs.obj]
external makeJSCoverageOptions :
(~resetOnNavigation: Js.boolean=?, unit) => jsCoverageOptions =
"";

This comment has been minimized.

@zploskey

zploskey Apr 6, 2018

Owner

Can we wrap these to convert from a bool as we have done elsewhere?

This comment has been minimized.

@jihchi

jihchi Apr 7, 2018

Author Collaborator

Fixed in commit d161884

Archie Lee added some commits Apr 6, 2018

Archie Lee
Change "startJSCoverage", "startCSSCoverage" options parameter to be …
…optional

Wrap "resetOnNavigation" to convert from a "bool"
exports.testPageContent = testPageContent;
exports.noSandbox = noSandbox;
/* getElementValueJs Not a pure module */
/* This output is empty. Its source's type definitions, externals and/or unused code got optimized away. */

This comment has been minimized.

@zploskey

zploskey Apr 8, 2018

Owner

We lost the generated js for the tests somehow.

This comment has been minimized.

@jihchi

jihchi Apr 9, 2018

Author Collaborator

Oops! Its weird!
Fixed in commit a65cec2

@jihchi

This comment has been minimized.

Copy link
Collaborator Author

jihchi commented Apr 10, 2018

@zploskey PR is done, could you please review it?

@zploskey
Copy link
Owner

zploskey left a comment

Looks good to me.

@zploskey zploskey merged commit a65cec2 into zploskey:master Apr 10, 2018

zploskey added a commit that referenced this pull request Apr 10, 2018

Merge PR #44 from jichi/bindings_for_coverage
Add bindings for Coverage
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment