Add multi-pass environment variable resolution#2322
Merged
Conversation
We were passing the root of the test folder instead of the unitTests specific folder into the runner, so it wasn't finding unit tests.
If a variable had env, config, or workspaceFolder as substring, it was being treated as an environment variable lookup on the reminder of the string minus the character immediately following the substring. For instance: envRoot was being looked up as "oot" in the configuarion environment block. The issue is that the regex to match the env., config., and workspaceFolder. patterns used . instead of \.
This gets rid of linting errors and adopts a more BDD literate assert style.
| "args": [ | ||
| "--extensionDevelopmentPath=${workspaceFolder}", | ||
| "--extensionTestsPath=${workspaceFolder}/out/test" | ||
| "--extensionTestsPath=${workspaceFolder}/out/test/unitTests" |
Contributor
There was a problem hiding this comment.
Oh, that's interesting...I guess we don't Launch Tests very much :)
Contributor
There was a problem hiding this comment.
Looks good so far. I'm not done reviewing yet...
Contributor
Author
|
@bobbrow Raised a good point in that this a lot of added complexity for nesting variable, and the case that this is a useful feature for customers is a nebulous prospect at best. With that in might, I pushed a change which simplifies this feature at the expense of nested variables. |
| newValue = process.env[name]; | ||
| } | ||
| break; | ||
| let regexp: () => RegExp = () => /\$\{((env|config|workspaceFolder)(\.|:))?(.*?)\}/g; |
sean-mcmanus
approved these changes
Jul 24, 2018
sean-mcmanus
left a comment
Contributor
There was a problem hiding this comment.
Seems good to me. Bob will probably want to finish reviewing as well.
Contributor
|
Do you know what's up with the Travis CI failure? |
bobbrow
approved these changes
Jul 24, 2018
As per microsoft/vscode-wordcount#5, running tests from CLI isn't supported with vscode except through the [workaround listed for Travis CI](https://code.visualstudio.com/docs/extensions/testing-extensions#_running-tests-automatically-on-travis-ci-build-machines).
Contributor
Author
|
These tests should pass in the future. I added the fix for Travis CI. |
john-patterson
added a commit
to john-patterson/vscode-cpptools
that referenced
this pull request
Jul 25, 2018
Realized I had left out the CHANGELOG update. For [PR 2322](microsoft#2322).
bobbrow
pushed a commit
that referenced
this pull request
Jul 27, 2018
Realized I had left out the CHANGELOG update. For [PR 2322](#2322).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Per issue #2057.
This allows a user to nest environment tags, or define tags based on the definition of other environment variables. If the result of a resolution still contains a
${...}tag, we will continue to scan the input until we reach a steady state where the variable expansion results in the same input.This should allow the following forms which were previously not possible in the environment definition: