Refactor node and browser tests

[ramya0820]

For more context refer to #2859

This PR is part 1 of refactoring node and browser tests.
Moved out environment variables, making them browser friendly and updated streamingReceiverSessions.ts file to use the new utils file.

If this looks fine, then coming up next (in maybe same or different PRs) are:

• Updating all node tests and pointing to use new reference
• Update browser tests (Since this should eliminate need for having separate browser tests, can consider modifying mocha scripts to run selected tests in browser using tagging or grep utility.)

[ramya-rao-a]

Left a few more comments to the contents of the environmentVariables.ts file. What do you think of renaming this file to testEnvVariables.ts ?

Usage wise, things are good. Therefore, the changes made to streamingReceiverSessions.ts can be ported over to the other test files in the same PR.

[ramya0820]

In latest commit:

• Refactored testUtils and separated out the AAD related utils. Note that, as before, cleaning namespace is not available for when running browser tests due to lack of support for AAD ([Service Bus] Support AAD authentication in Browser #2556) Defined new internal module aadUtils and marked it as external for browser rollup. Added AAD code with 'isNode' guard for tree-shaking it out in browser rollup. cc @bterlson
• Verified that both node and browser tests are running as expected.
• Included package.json scripts update to see if we can trigger things in CI and verify
• Used mocha's grep and #RunInBrowser hashtag to select tests that will be run in browser. This is as per what we had finalized in [Service Bus] Browser Tests for Service Bus #2163

Please take a look @bterlson @ramya-rao-a @AlexGhiondea @sadasant

[bsiegel]

/azp run azure-sdk-for-js - all-tests - servicebus

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[bterlson]

@ramya0820 talked with @ramya-rao-a as well as elements of our cosmos team (@southpolesteve) and it seems like bundling + nested package.json is not a good approach. Steve suggests possibly using a script. However, we want to do something simpler in this PR - revert the change importing from package.json, and doing the "proper" browserification, something like:

package.json
Add the following mapping to the browser field:
"./dist-esm/test/utils/aadUtils.js": "./dist-esm/test/utils/aadUtils.browser.js"

aadUtils.browser.ts
Add a file next to aadUtils.ts with the following contents:

export async function recreateQueue(): Promise<void> {}
export async function recreateSubscription(): Promise<void> {}
export async function recreateTopic(): Promise<void> {}
export const loginWithServicePrincipalSecret = 0;

aadUtils.ts
As this file is now just for node, you can remove all the runtime checks.

Let me know if you have questions about this approach!

[ramya0820]

@ramya0820 talked with @ramya-rao-a as well as elements of our cosmos team (@southpolesteve) and it seems like bundling + nested package.json is not a good approach. Steve suggests possibly using a script. However, we want to do something simpler in this PR - revert the change importing from package.json, and doing the "proper" browserification, something like:

package.json
Add the following mapping to the browser field:
"./dist-esm/test/utils/aadUtils.js": "./dist-esm/test/utils/aadUtils.browser.js"

aadUtils.browser.ts
Add a file next to aadUtils.ts with the following contents:

export async function recreateQueue(): Promise<void> {}
export async function recreateSubscription(): Promise<void> {}
export async function recreateTopic(): Promise<void> {}
export const loginWithServicePrincipalSecret = 0;

aadUtils.ts
As this file is now just for node, you can remove all the runtime checks.

Let me know if you have questions about this approach!

@bterlson I had made changes as suggested (since the main difference seems to be in the paths used in the browser field). However, it still fails with error -
"Cannot read property 'Stream' of undefined"
It looks like the package.json is still isn't mapping the paths correctly as the imports are being pulled in from the non .browser file even when run in browser mode. (Just like what I had encountered when trying this previously as well (#3017 (comment)) )
(I even tried adding "buffer": "buffer", "stream": "stream-browserify" entries to the browser field, still no luck)
How would npm and package.json determine to use the browser fields? Because the difference in npm script we run is based on flag ONLY_BROWSER which is something we define, correct?

Also, the findings seem to be inline with conversation we had earlier about use of 'browser' field having an issue and that there may be a bug in rollup-plugin-node-resolve? You mentioned that you would like to test with webpack/parcel/pika to see whether other bundlers hit this issue?
Were these verified as such? If so, would it be possible to push those working changes to the PR? Because currently, it still doesn't seem to be working.

[bterlson]

I checked Webpack, and it wasn't affected but I think by chance. I came to the conclusion that it's just too risky to have a package.json outside the package root. It's like a honeypot for tools to get broken by!

I'll look at the stream errors later today. It's a little suspicious that this wasn't an issue when externalizing ms-rest-js since stream is used elsewhere IIRC.

Bundlers use the browser field if the field is present and the user has indicated they are building for the browser (which I think is default with webpack). npm and node don't understand that field, it's just for tools.

[ramya0820]

I checked Webpack, and it wasn't affected but I think by chance. I came to the conclusion that it's just too risky to have a package.json outside the package root. It's like a honeypot for tools to get broken by!

I'll look at the stream errors later today. It's a little suspicious that this wasn't an issue when externalizing ms-rest-js since stream is used elsewhere IIRC.

Bundlers use the browser field if the field is present and the user has indicated they are building for the browser (which I think is default with webpack). npm and node don't understand that field, it's just for tools.

Oh but we have just one package.json file and that is at the root 'service-bus' folder right?
Did you mean to have another one within tests folder?

About externalizing ms-rest-js, I believe this is in reference to amqp-common.
It looks like ms-rest-js is marked as external for it.
But if I understand correctly, we don't want to do the same for ms-rest-nodeauth here and try to rely on browser scripts only?
If we don't mark it as external, then too it throws a 'loginWithServicePrincipalSecret' cannot be on undefined, pointing to the root cause that the mapping provided in browser field isn't working as expected. (Because the aadUtils non-browser version file is the only one that tries to access this and that shouldn't be one to get referenced in browser rollup).

Got it, overall it's unclear as to how the rollup tool would use the browser field from package.json as that seems to maybe affect for when package is used by say dependencies. Will look into rollup internals and see if anything useful hits up.

You mentioned manually changing the .js file to map to .browser version had worked? Did you mean it worked by resolving the buffer issues noted in #3119 ? Or is that unrelated, and you had verified using npm only ? @bterlson
I think @KarishmaGhiya was looking for clarity on this as well.

[ramya0820]

I checked Webpack, and it wasn't affected but I think by chance. I came to the conclusion that it's just too risky to have a package.json outside the package root. It's like a honeypot for tools to get broken by!
I'll look at the stream errors later today. It's a little suspicious that this wasn't an issue when externalizing ms-rest-js since stream is used elsewhere IIRC.
Bundlers use the browser field if the field is present and the user has indicated they are building for the browser (which I think is default with webpack). npm and node don't understand that field, it's just for tools.

Oh but we have just one package.json file and that is at the root 'service-bus' folder right?
Did you mean to have another one within tests folder?

About externalizing ms-rest-js, I believe this is in reference to amqp-common.
It looks like ms-rest-js is marked as external for it.
But if I understand correctly, we don't want to do the same for ms-rest-nodeauth here and try to rely on browser scripts only?
If we don't mark it as external, then too it throws a 'loginWithServicePrincipalSecret' cannot be on undefined, pointing to the root cause that the mapping provided in browser field isn't working as expected. (Because the aadUtils non-browser version file is the only one that tries to access this and that shouldn't be one to get referenced in browser rollup).

Got it, overall it's unclear as to how the rollup tool would use the browser field from package.json as that seems to maybe affect for when package is used by say dependencies. Will look into rollup internals and see if anything useful hits up.

You mentioned manually changing the .js file to map to .browser version had worked? Did you mean it worked by resolving the buffer issues noted in #3119 ? Or is that unrelated, and you had verified using npm only ? @bterlson
I think @KarishmaGhiya was looking for clarity on this as well.

Discussed offline with @ramya-rao-a, bits were clarified about updating constants.js file and use of json() plugin.

Turns out rollup json plugin was still needed per error:

dist-esm/test/*.spec.js → test-browser/index.js...
[!] Error: Unexpected token (Note that you need rollup-plugin-json to import JSON files)
node_modules\adal-node\package.json (2:9)
1: {
2:   "_args": [
            ^
3:     [
4:       "adal-node@0.1.28",
Error: Unexpected token (Note that you need rollup-plugin-json to import JSON files)
    at error (c:\workspace\newrepo609\azure-sdk-for-js\sdk\servicebus\service-bus\node_modules\rollup\dist\rollup.js:9236:30)
    at Module.error (c:\workspace\newrepo609\azure-sdk-for-js\sdk\servicebus\service-bus\node_modules\rollup\dist\rollup.js:12909:9)
    at tryParse (c:\workspace\newrepo609\azure-sdk-for-js\sdk\servicebus\service-bus\node_modules\rollup\dist\rollup.js:12825:16)
    at Module.setSource (c:\workspace\newrepo609\azure-sdk-for-js\sdk\servicebus\service-bus\node_modules\rollup\dist\rollup.js:13102:33)
    at Promise.resolve.catch.then.then.then (c:\workspace\newrepo609\azure-sdk-for-js\sdk\servicebus\service-bus\node_modules\rollup\dist\rollup.js:15924:20)

And on restoring it's use, the file mapping still seems to pull in the aadUtils meant for node.

@bterlson @ramya-rao-a @AlexGhiondea

[ramya-rao-a]

the file mapping still seems to pull in the aadUtils meant for node.

We troubleshooted this offline. What was missing was removing of the stale package.json file lying around in the dist-esm folder. It is the existence of this file that breaks the mapping we want to have in the browser field.

[ramya0820]

the file mapping still seems to pull in the aadUtils meant for node.

We troubleshooted this offline. What was missing was removing of the stale package.json file lying around in the dist-esm folder. It is the existence of this file that breaks the mapping we want to have in the browser field.

Yes, @bterlson could you take a look at updated changes?

Also, the npm vs rush issue still exists and seems independent of these changes. @KarishmaGhiya

[ramya-rao-a]

@ramya0820 One last change:

Please remove the dist-esm/package.json entry in the files section in the package.json file.
This was added when we imported package.json file in the constants.ts. Now that we have removed this import, we don't need to ship the package.json file from the files section

@bterlson has logged #3224 as a follow up item to the removal of package.json import. Please pick that up as your next item so that we avoid falling into the case of shipping a new update but forgetting to update the constants file

[bterlson]

Package.json and rollup config files look great! Nice work!

@bsiegel may have already checked the pipeline yaml changes in this PR, but just want to make sure that they are expected here!

[KarishmaGhiya]

Great! Since all the other issues are fixed, I will now safely investigate into the issue you are running into with the rush workflow @ramya0820

[ramya0820]

@ramya0820 One last change:

Please remove the dist-esm/package.json entry in the files section in the package.json file.
This was added when we imported package.json file in the constants.ts. Now that we have removed this import, we don't need to ship the package.json file from the files section

@bterlson has logged #3224 as a follow up item to the removal of package.json import. Please pick that up as your next item so that we avoid falling into the case of shipping a new update but forgetting to update the constants file

Oops, just seeing this comment - Will update the files field as part of continued efforts for #3119.
I did want to touch upon the solution that we have arrived in that the presence of package.json interferes with node module resolution:

• As part of troubleshooting we mainly did the npm run clean as opposed to rebuild which confirms that the presence of package.json was instrumental in blocking the file resolution. @bterlson @ramya-rao-a could you share more details that explain how the presence of this file is problematic and when exactly does the browser field entries get resolved? I think this might help with [Service Bus] Issue with browser tests being run on CI #3119 investigation which seems to center around resolution of the 'Buffer' module.

[ramya-rao-a]

... which confirms that the presence of package.json was instrumental in blocking the file resolution. @bterlson @ramya-rao-a could you share more details that explain how the presence of this file is problematic

@ramya0820 Below is from the email that you started for the build failures in this PR where @bterlson talks about how the presence of package.json file in the dist-esm folder causes problems in the mapping. Sharing it here for the wider audience

I assumed a simple entry mapping dist-esm/test/utils/aadUtils.js 
to dist-esm/test/utils/aadUtils.browser.js would work, but guess what: it didn’t ☹

Found it due to the nested package.json file under dist-esm which got added recently when 
we added json imports for getting the package version. Rollup will find this nested 
package.json first when looking for browser maps, and since that package.json is now in the 
dist-esm folder, the mapping is broken – it’s attempting to map 
`dist-esm/dist-esm/test/utils/aadUtils.js` which of course doesn’t exist.

Based on the above, we concluded that removing the json imports that was placed to get the package name and version should resolve the problem with the mapping

when exactly does the browser field entries get resolved

Like @bterlson mentioned in #3017 (comment) the browser field is used by bundlers. In our case, the rollup tool.

The mappings in the browser field is used by the rollup-plugin-node-resolve plugin based on what is provided in the mainFields property. Please read the usage of the node-resolve plugin to learn more

[ramya0820]

Got it, thanks @ramya-rao-a!
Noting to self that it is overall not a good idea to package package.json into build output as I suppose it is more like a configuration file and not meant to be used as source.