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

amp-pixel: Minor test improvements #26918

Merged
merged 2 commits into from
Feb 23, 2020
Merged

Conversation

mdmower
Copy link
Contributor

@mdmower mdmower commented Feb 23, 2020

Resolve sinon warnings:

ERROR: 'The <amp-pixel> src attribute must start with "https://" or "//". Invalid value: http://pubads.g.doubleclick.net/activity;dc_iu=1/abc;ord=2'
    The test "amp-pixel   should disallow http URLs" resulted in a call to console.error. (See above line.)
    ⤷ If the error is not expected, fix the code that generated the error.
    ⤷ If the error is expected (and synchronous), use the following pattern to wrap the test code that generated the error:
        'allowConsoleError(() => { <code that generated the error> });'
    ⤷ If the error is expected (and asynchronous), use the following pattern at the top of the test:
        'expectAsyncConsoleError(<string or regex>[, <number of times the error message repeats>]);'
ERROR: 'The <amp-pixel> src attribute must start with "https://" or "//". Invalid value: /activity;dc_iu=1/abc;ord=2'
    The test "amp-pixel   should disallow relative URLs" resulted in a call to console.error. (See above line.)
    ⤷ If the error is not expected, fix the code that generated the error.
    ⤷ If the error is expected (and synchronous), use the following pattern to wrap the test code that generated the error:
        'allowConsoleError(() => { <code that generated the error> });'
    ⤷ If the error is expected (and asynchronous), use the following pattern at the top of the test:
        'expectAsyncConsoleError(<string or regex>[, <number of times the error message repeats>]);'
ERROR: 'The <amp-pixel> src attribute must start with "https://" or "//". Invalid value: https/activity;dc_iu=1/abc;ord=2'
    The test "amp-pixel   should disallow fake-protocol URLs" resulted in a call to console.error. (See above line.)
    ⤷ If the error is not expected, fix the code that generated the error.
    ⤷ If the error is expected (and synchronous), use the following pattern to wrap the test code that generated the error:
        'allowConsoleError(() => { <code that generated the error> });'
    ⤷ If the error is expected (and asynchronous), use the following pattern at the top of the test:
        'expectAsyncConsoleError(<string or regex>[, <number of times the error message repeats>]);'

Full log: https://gist.github.com/mdmower/d2252c6dc06d096687c56a6368a30c41

Expect console errors.
@@ -99,20 +99,23 @@ describes.realWin('amp-pixel', {amp: true}, env => {
});

it('should disallow http URLs', () => {
expectAsyncConsoleError(/src attribute must start with/);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit, can reuse the rejectedWith content by creating a single const reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review. This change has been made.

@kristoferbaxter
Copy link
Contributor

LGTM, might want to consider the nit.

Create reusable regex for tests.
@mdmower mdmower merged commit c1d233f into ampproject:master Feb 23, 2020
robinvanopstal added a commit to jungvonmatt/amphtml that referenced this pull request Feb 24, 2020
* master: (41 commits)
  custom-element: Minor test improvements (ampproject#26923)
  amp-pixel: Minor test improvements (ampproject#26918)
  viewer: Minor test improvements (ampproject#26906)
  dom: Minor test improvements (ampproject#26913)
  amp-action: Support whitelist lookup in AmpDocShadow (ampproject#26684)
  ✨ Update amp-access-scroll (ampproject#26810)
  🚀 Remove doc css and base css from ESM build (ampproject#26889)
  📖 [amp-story-player] Initial docs (ampproject#26606)
  Amp consent restrict fullscreen prod flag (ampproject#26909)
  📖 Clarify SXG duration minimum (ampproject#26890)
  Improve test vendor requests macros (ampproject#26828)
  🚀 Move scroll left and top macros out of url-replacement-impl (ampproject#25594)
  Update consent string maximum size to 200 bytes (ampproject#26741)
  ✨[amp-story-player] Adds tap-to-next/previous story (ampproject#26865)
  update owners file with correct syntax (ampproject#26899)
  amp-sticky-ad: Fix unit test (ampproject#26855)
  Add performance metrics to README (ampproject#26891)
  🐛 Bug fix: check links test (ampproject#26739)
  ✨Idealmedia uniq ad (ampproject#25838)
  📦 Update dependency jsdom to v16.2.0 (ampproject#26591)
  ...
@mdmower mdmower deleted the pr-test4 branch March 16, 2020 01:53
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

4 participants