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

Remove fake "browser" and "dom" modules #3106

Merged
merged 6 commits into from
Sep 1, 2016
Merged

Remove fake "browser" and "dom" modules #3106

merged 6 commits into from
Sep 1, 2016

Conversation

lucaswoj
Copy link
Contributor

@lucaswoj lucaswoj commented Aug 31, 2016

This PR removes the fake "browser" and "dom" modules used in the test environment in favor of using the real modules with jsdom.

This is a follow-up to #3063

Checklist

  • briefly describe the changes in this PR
  • 🍏
  • get a PR review 👍 / 👎

@@ -1,29 +1,46 @@
'use strict';

/**
* Unlike js/util/browser.js, this code is written with the expectation
Copy link
Contributor

Choose a reason for hiding this comment

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

This description was specific to the browser version of browser.js, and can be removed or rewritten.

@jfirebaugh
Copy link
Contributor

Can we merge this as-is and go for canvas, ajax, and web_worker in a followup PR? I'd like to write some Popup tests that require these changes.

@lucaswoj
Copy link
Contributor Author

@jfirebaugh Sure! I'll fix this up and merge shortly.

@jfirebaugh
Copy link
Contributor

The coverage dropped because the test-suite didn't run. This error did not fail the build:

/Users/distiller/mapbox-gl-js/test/suite_implementation.js:16
    browser.devicePixelRatio = options.pixelRatio;
                             ^

TypeError: Cannot set property devicePixelRatio of #<Object> which has only a getter
    at module.exports (/Users/distiller/mapbox-gl-js/test/suite_implementation.js:16:30)
    at /Users/distiller/mapbox-gl-js/node_modules/mapbox-gl-test-suite/lib/render.js:62:9
    at /Users/distiller/mapbox-gl-js/node_modules/mapbox-gl-test-suite/lib/harness.js:78:17
    at start (/Users/distiller/mapbox-gl-js/node_modules/mapbox-gl-test-suite/node_modules/queue-async/build/queue.js:35:43)
    at Server.<anonymous> (/Users/distiller/mapbox-gl-js/node_modules/mapbox-gl-test-suite/node_modules/queue-async/build/queue.js:48:24)
    at Server.g (events.js:260:16)
    at emitNone (events.js:67:13)
    at Server.emit (events.js:166:7)
    at emitListeningNT (net.js:1276:10)
    at nextTickCallbackWith1Arg (node.js:431:9)

@lucaswoj
Copy link
Contributor Author

Test suite is failing because no fill outlines are being drawn but CI is still passing. This is a fun one 😂

@jfirebaugh
Copy link
Contributor

set -e doesn't exit if the command that fails is part of a && list. 😢

@lucaswoj
Copy link
Contributor Author

lucaswoj commented Sep 1, 2016

Easy fix! We weren't using the &&.

@lucaswoj
Copy link
Contributor Author

lucaswoj commented Sep 1, 2016

Force pushed to resolve merge conflicts. This should be 🍏 and ready to 🚢 .

@lucaswoj lucaswoj merged commit c24473f into master Sep 1, 2016
@lucaswoj lucaswoj deleted the test-dom branch September 1, 2016 22:17
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.

None yet

2 participants