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

Fix #3576 import redirects. Replace native-request with needle. #3582

Merged
merged 2 commits into from Jan 3, 2021

Conversation

zaquest
Copy link
Contributor

@zaquest zaquest commented Dec 29, 2020

Quick overview of the changes:

  1. Replace native-request with needle - hopefully small enough http client to not be a burden.
  2. Add nock based test to verify correct work of the redirect.
  3. Moved logger.addListener call outside of lessTester factory function in less/test/less-test.js. This was necessary to avoid registering the same listeners twice as for the test it was necessary to instantiate lessTester one more time.
  4. Created one more instance of lessTester in less/test/index.js. This is necessary since tests in the test suite are not isolated and leave less instance in the state in which remote imports are completely broken.

@zaquest
Copy link
Contributor Author

zaquest commented Dec 29, 2020

Hmm, apprently nock doesn't work with node 8.

@zaquest
Copy link
Contributor Author

zaquest commented Dec 31, 2020

Btw, I noticed that on AppVeyor all builds are done with node version 8 and from the environment names seems like the objective was to test with different node versions.

@@ -0,0 +1,3 @@
@import "https://example.com/redirect.less";
Copy link
Member

Choose a reason for hiding this comment

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

Is this something that will always redirect? As in, will this test always succeed with this remote endpoint?

Copy link
Contributor Author

@zaquest zaquest Jan 1, 2021

Choose a reason for hiding this comment

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

This is the url nock mocks, so node doesn't actually send the request to a remote server. So as long as nock is set up to intercept this exact url it will always redirect.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, perfect.

(() => {
// Create new tester, since tests are not independent and tests
// above modify tester in a way that breaks remote imports.
lessTester = lessTest();
Copy link
Member

Choose a reason for hiding this comment

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

If remote imports are being broken between tests, where do you think would be the best place to fix this? Is this a problem in Less itself? Or a problem in the way tests are set up?

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 think the problem is in the way tests are set up. Ideally tests should be independent, meaning each test should have it's own instance of less, even more so since the less instance is modified by some tests. Or, alternatively, each test should have a tear down part, where it returns less instance to it's initial (before test) state. However, if I understand correctly tests are also run concurrently, which might be a problem for the set up / tear down approach.

I suspect the reason remote imports get broken is because of plugin tests, might be wrong here, haven't actually investigated it.

One way to approach this is to define a function with contents of less-node/index.js and call it in less-node/index.js and before each test.

Copy link
Member

Choose a reason for hiding this comment

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

I don't believe tests are run concurrently. They're run in a queue. But definitely it's possible they mutate the less object or don't return it to previous state. However, I doubt that's intentional, so it would be great to track it down, but maybe that's out of the scope of this issue.

@matthew-dean
Copy link
Member

@zaquest

Btw, I noticed that on AppVeyor all builds are done with node version 8 and from the environment names seems like the objective was to test with different node versions.

This may be an oversight. Node versions have been incremented over time to support, I think, the last 4 major Node versions. Environment vars may not hav been kept in sync.

Thanks for the PR! I left some comments / questions on it.

@@ -0,0 +1,3 @@
@import "https://example.com/redirect.less";
Copy link
Member

Choose a reason for hiding this comment

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

Ah, perfect.

@matthew-dean matthew-dean merged commit ec74875 into less:master Jan 3, 2021
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