Fix build:test script

[ramya0820]

This PR addresses build failures for live-tests.

Closes #2163

[ramya-rao-a]

Apologies if this was already covered in the previous PR that had the browser tests, but why arent we using the rollup.test.config.js in build:test:browser ?
If we did that and uncommented the browser part in rollup.test.config.js file, I believe we dont need the new env variable BROWSER_TEST

[ramya0820]

Apologies if this was already covered in the previous PR that had the browser tests, but why arent we using the rollup.test.config.js in build:test:browser ?
If we did that and uncommented the browser part in rollup.test.config.js file, I believe we dont need the new env variable BROWSER_TEST

Yes, that wouldn't work because the test.config directly instantiates nodeConfig(). We'll need to introduce a flag to differentiate from browser.

[sadasant]

Looks good to me (but I haven't tried it)

[ramya-rao-a]

Yes, that wouldn't work because the test.config directly instantiates nodeConfig(). We'll need to introduce a flag to differentiate from browser.

So does the other config. Below is the contents of rollup.config.js.
The first if checks would succeed and nodeConfig() will get instantiated.

const inputs = [];

if (!process.env.ONLY_BROWSER) {
  inputs.push(base.nodeConfig());
}

if (!process.env.ONLY_NODE) {
  inputs.push(base.browserConfig());
  inputs.push(base.browserConfig({ production: true }));
}

if (process.env.BROWSER_TEST) {
  inputs.push(base.browserConfig({ test: true }));
}

export default inputs;

My concern here is 2 fold

• introduction of a new variable BROWSER_TEST when we can use the existing ones instead (NODE_ONLY and BROWSER_ONLY)
• not using the rollup.test.config.js which was created especially for the test scenario

If we update rollup.test.config.js to use similar if checks as we do in rollup.config.js (like above), I believe we can use the existing env variables and not introduce new BROWSER_TEST

Side note: I know that BROWSER_TEST was not introduced in this PR. I am using the word "introduce" with reference to the browser tests

[ramya0820]

Yes, that wouldn't work because the test.config directly instantiates nodeConfig(). We'll need to introduce a flag to differentiate from browser.

So does the other config. Below is the contents of rollup.config.js.
The first if checks would succeed and nodeConfig() will get instantiated.

const inputs = [];

if (!process.env.ONLY_BROWSER) {
  inputs.push(base.nodeConfig());
}

if (!process.env.ONLY_NODE) {
  inputs.push(base.browserConfig());
  inputs.push(base.browserConfig({ production: true }));
}

if (process.env.BROWSER_TEST) {
  inputs.push(base.browserConfig({ test: true }));
}

export default inputs;

My concern here is 2 fold

• introduction of a new variable BROWSER_TEST when we can use the existing ones instead (NODE_ONLY and BROWSER_ONLY)
• not using the rollup.test.config.js which was created especially for the test scenario

If we update rollup.test.config.js to use similar if checks as we do in rollup.config.js (like above), I believe we can use the existing env variables and not introduce new BROWSER_TEST

Side note: I know that BROWSER_TEST was not introduced in this PR. I am using the word "introduce" with reference to the browser tests

Got it, yes updating the test.config to reuse the NODE_ONLY, BROWSER_ONLY flags would get us same result and makes sense.

[ramya-rao-a]

The changes look good, but we now we have a new set of failures:

[ramya0820]

Closing this in favor of #3017 as enabling of scripts are being tested there.

[ramya-rao-a]

@ramya0820 I believe the changes in this PR are very much valid regardless of the latest type of CI failures. #3017 doesnt have any of the changes we discussed in this PR i.e removal of BROWSER_TEST and using rollup.test.config.js for building browser tests.

Please re-open this PR.
We can have this merged to get the build step of browser steps resolved. Since we have disabled the running of browser tests in #3084, this PR can be merged

[ramya0820]

@ramya0820 I believe the changes in this PR are very much valid regardless of the latest type of CI failures. #3017 doesnt have any of the changes we discussed in this PR i.e removal of BROWSER_TEST and using rollup.test.config.js for building browser tests.

Please re-open this PR.
We can have this merged to get the build step of browser steps resolved. Since we have disabled the running of browser tests in #3084, this PR can be merged

Got it, this is pretty much refactoring of the rollup scripts and would belong to #2859
As discussed on other thread, since CI related failures seem related to rollup and browser tests related code, they will need the latest changes finalized. I've created #3119 to track the CI related item so it can be moved out of development scope for now.