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

Fixed and unskipped some amp-story tests #16977

Merged
merged 5 commits into from Jul 20, 2018

Conversation

addieachan
Copy link
Contributor

@addieachan
Copy link
Contributor Author

I tried deleting a test that I thought was deprecated (part of system layer and couldn't find method anymore), but I may have done so wrongfully if I just misunderstood it!

@addieachan addieachan changed the title Fixed/Re-enabled two tests and deleted one Fixed/Re-enabled previously skipped amp-story tests Jul 20, 2018
@Enriqe Enriqe requested review from newmuis and Enriqe July 20, 2018 18:29
@Enriqe Enriqe added this to the Fixit Summit July 2018 milestone Jul 20, 2018
@addieachan addieachan changed the title Fixed/Re-enabled previously skipped amp-story tests Fixed and unskipped some amp-story tests Jul 20, 2018
@@ -619,4 +608,4 @@ describes.realWin('amp-story origin whitelist', {
story.originWhitelist_ = ['example.co'];
expect(story.isOriginWhitelisted_('https://example.co.uk')).to.be.false;
});
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Re-add newline at end of file

expect(pauseOldPageStub).to.have.been.calledOnce;
expect(resumeNewPageStub).to.have.been.calledOnce;
});
});
});

// TODO(#11639): Re-enable this test.
Copy link
Contributor

Choose a reason for hiding this comment

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

could you also remove this comment please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually haven't figured out that one yet, so I was supposed to skip it as well instead of accidentally enabling it again. Sorry for the confusion! I readded the skip so the comment makes sense

@codecov-io
Copy link

codecov-io commented Jul 20, 2018

Codecov Report

Merging #16977 into master will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #16977      +/-   ##
==========================================
+ Coverage   78.11%   78.13%   +0.02%     
==========================================
  Files         561      562       +1     
  Lines       40760    40782      +22     
==========================================
+ Hits        31840    31867      +27     
+ Misses       8920     8915       -5
Flag Coverage Δ
#integration_tests 36.17% <ø> (-0.02%) ⬇️
#unit_tests 77.18% <ø> (+0.02%) ⬆️
Impacted Files Coverage Δ
extensions/amp-next-page/0.1/next-page-service.js 74.26% <0%> (-3.51%) ⬇️
...nsions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js 83.33% <0%> (-0.44%) ⬇️
3p/ampcontext.js 92.63% <0%> (ø) ⬆️
src/utils/array.js 100% <0%> (ø) ⬆️
extensions/amp-carousel/0.1/slidescroll.js 87.27% <0%> (ø) ⬆️
.../bookend/components/bookend-component-interface.js 80% <0%> (ø)
src/custom-element.js 94.24% <0%> (+0.01%) ⬆️
extensions/amp-selector/0.1/amp-selector.js 97.18% <0%> (+0.1%) ⬆️
extensions/amp-story/0.1/amp-story.js 50.09% <0%> (+0.19%) ⬆️
extensions/amp-story/0.1/amp-story-page.js 55.88% <0%> (+0.49%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a8f25b...af1b455. Read the comment docs.

@newmuis newmuis merged commit 43f00e5 into ampproject:master Jul 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants