From e948fe47893b738aeb9db7fe23697a2ffd1542f9 Mon Sep 17 00:00:00 2001 From: Pedro Yamada Date: Tue, 17 Sep 2024 07:20:34 +0000 Subject: [PATCH 01/10] Make changes to have all tests run always This changes the v2/v3 test setup so that tests that are skipped for V3 run. A subsequent change will improve our logging to automatically diff the outputs of both versions in a single process. The idea here is to easily identify tests that are green but skipped, as well as get a grasp of current failures. --- packages/core/test-utils/src/utils.js | 141 +++++++++++++++++++------- 1 file changed, 102 insertions(+), 39 deletions(-) diff --git a/packages/core/test-utils/src/utils.js b/packages/core/test-utils/src/utils.js index 3acc781c2..36ea76c9f 100644 --- a/packages/core/test-utils/src/utils.js +++ b/packages/core/test-utils/src/utils.js @@ -11,16 +11,17 @@ import type { PackagedBundle, } from '@atlaspack/types'; import type {FileSystem} from '@atlaspack/fs'; +import {MemoryFS, ncp as _ncp, NodeFS, OverlayFS} from '@atlaspack/fs'; import type WorkerFarm from '@atlaspack/workers'; import type {IncomingMessage} from 'http'; +import http from 'http'; import invariant from 'assert'; +import assert from 'assert'; import util from 'util'; import Parcel, {createWorkerFarm} from '@atlaspack/core'; -import assert from 'assert'; import vm from 'vm'; import v8 from 'v8'; -import {NodeFS, MemoryFS, OverlayFS, ncp as _ncp} from '@atlaspack/fs'; import path from 'path'; import url from 'url'; import WebSocket from 'ws'; @@ -28,7 +29,6 @@ import nullthrows from 'nullthrows'; import {parser as postHtmlParse} from 'posthtml-parser'; import postHtml from 'posthtml'; import EventEmitter from 'events'; -import http from 'http'; import https from 'https'; import {makeDeferredWithPromise, normalizeSeparators} from '@atlaspack/utils'; @@ -712,7 +712,10 @@ function prepareBrowserContext( WebSocket, TextEncoder, TextDecoder, - console: {...console, clear: () => {}}, + console: { + ...console, + clear: () => {}, + }, location: { hostname: 'localhost', origin: 'http://localhost', @@ -863,6 +866,7 @@ function prepareWorkerContext( } const nodeCache = new Map(); + // no filepath = ESM function prepareNodeContext( filePath, @@ -983,6 +987,7 @@ function prepareNodeContext( } let instanceId = 0; + export async function runESM( baseDir: FilePath, entries: Array<[string, string]>, @@ -993,6 +998,7 @@ export async function runESM( ): Promise> { let id = instanceId++; let cache = new Map(); + function load(inputSpecifier, referrer, code = null) { // ESM can request bundles with an absolute URL. Normalize this to the baseDir. let specifier = inputSpecifier.replace('http://localhost', baseDir); @@ -1081,6 +1087,7 @@ export async function runESM( } let entryPromises = new Map(); + function entry(specifier, referrer, code) { let m = load(specifier, referrer, code); let promise = entryPromises.get(m); @@ -1267,89 +1274,145 @@ export function request( // $FlowFixMe let origDescribe = globalThis.describe; -let parcelVersion: string | void; +let testVersionContext: string | void; + export function describe(...args: mixed[]) { - parcelVersion = undefined; origDescribe.apply(this, args); } describe.only = function (...args: mixed[]) { - parcelVersion = undefined; origDescribe.only.apply(this, args); }; describe.skip = function (...args: mixed[]) { - parcelVersion = undefined; origDescribe.skip.apply(this, args); }; describe.v2 = function (...args: mixed[]) { - parcelVersion = 'v2'; - if (!isAtlaspackV3) { - origDescribe.apply(this, args); - } + testVersionContext = 'v2'; + origDescribe.apply(this, args); }; describe.v2.only = function (...args: mixed[]) { - parcelVersion = 'v2'; - if (!isAtlaspackV3) { - origDescribe.only.apply(this, args); - } + testVersionContext = 'v2'; + origDescribe.only.apply(this, args); }; describe.v3 = function (...args: mixed[]) { - parcelVersion = 'v3'; + testVersionContext = 'v3'; if (isAtlaspackV3) { origDescribe.apply(this, args); } }; describe.v3.only = function (...args: mixed[]) { - parcelVersion = 'v3'; + testVersionContext = 'v3'; if (isAtlaspackV3) { origDescribe.only.apply(this, args); } }; let origIt = globalThis.it; -export function it(...args: mixed[]) { + +type TestCase = () => void | Promise; + +function itImpl( + only: boolean, + title: string, + builder: TestCase, + ...args: mixed[] +) { + const fn = only ? origIt.only : origIt; + console.log({testVersionContext, isAtlaspackV3, only, title}); if ( - parcelVersion == null || - (parcelVersion == 'v2' && !isAtlaspackV3) || - (parcelVersion == 'v3' && isAtlaspackV3) + testVersionContext == null || + (testVersionContext === 'v2' && !isAtlaspackV3) || + (testVersionContext === 'v3' && isAtlaspackV3) ) { - origIt.apply(this, args); + fn.call(this, title, builder, ...args); + } + if (testVersionContext === 'v2' && isAtlaspackV3) { + fn.call( + this, + title, + async function () { + try { + await builder.call(this); + } catch (err) { + console.error(`TEST: ${title} failed for V3`, err); + return; + } + console.error(`TEST: ${title} is green for V3 but skipped`); + }, + args, + ); } } -it.only = function (...args: mixed[]) { - origIt.only.apply(this, args); +export function it( + title: string, + builder: () => void | Promise, + ...args: mixed[] +) { + itImpl.call(this, false, title, builder, ...args); +} + +it.only = function (title: string, builder: TestCase, ...args: mixed[]) { + itImpl.call(this, true, title, builder, ...args); }; -it.skip = function (...args: mixed[]) { - origIt.skip.apply(this, args); +it.skip = function ( + title: string, + builder: () => void | Promise, + ...args: mixed[] +) { + origIt.skip.call(this, title, builder, ...args); }; -it.v2 = function (...args: mixed[]) { - if (!isAtlaspackV3) { - origIt.apply(this, args); - } +it.v2 = function ( + title: string, + builder: () => void | Promise, + ...args: mixed[] +) { + const previous = testVersionContext; + testVersionContext = 'v2'; + itImpl.call(this, false, title, builder, ...args); + testVersionContext = previous; }; -it.v2.only = function (...args: mixed[]) { - if (!isAtlaspackV3) { - origIt.only.apply(this, args); - } +it.v2.only = function ( + title: string, + builder: () => void | Promise, + ...args: mixed[] +) { + const previous = testVersionContext; + testVersionContext = 'v2'; + itImpl.call(this, true, title, builder, ...args); + testVersionContext = previous; }; -it.v3 = function (...args: mixed[]) { +it.v3 = function ( + title: string, + builder: () => void | Promise, + ...args: mixed[] +) { + const previous = testVersionContext; + testVersionContext = 'v3'; if (isAtlaspackV3) { - origIt.apply(this, args); + itImpl.call(this, false, title, builder, ...args); } + testVersionContext = previous; }; -it.v3.only = function (...args: mixed[]) { +it.v3.only = function ( + title: string, + builder: () => void | Promise, + ...args: mixed[] +) { + const previous = testVersionContext; + testVersionContext = 'v3'; if (isAtlaspackV3) { - origIt.only.apply(this, args); + itImpl.call(this, true, title, builder, ...args); } + testVersionContext = previous; }; From 016ddd11e9da3da9d3728cb41f806f6d42fcfedc Mon Sep 17 00:00:00 2001 From: Pedro Yamada Date: Tue, 17 Sep 2024 07:31:23 +0000 Subject: [PATCH 02/10] Patch test utils to remove console.log on itImpl --- packages/core/test-utils/src/utils.js | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/core/test-utils/src/utils.js b/packages/core/test-utils/src/utils.js index 36ea76c9f..e8da59ca1 100644 --- a/packages/core/test-utils/src/utils.js +++ b/packages/core/test-utils/src/utils.js @@ -1323,7 +1323,6 @@ function itImpl( ...args: mixed[] ) { const fn = only ? origIt.only : origIt; - console.log({testVersionContext, isAtlaspackV3, only, title}); if ( testVersionContext == null || (testVersionContext === 'v2' && !isAtlaspackV3) || From 7ff8b043c346c7180e9f56ddb5c92ec530d3a962 Mon Sep 17 00:00:00 2001 From: Pedro Yamada Date: Tue, 17 Sep 2024 07:32:21 +0000 Subject: [PATCH 03/10] Don't panic on JavaScript resolver This allows us to run integration tests that fail without crashing the process. --- crates/atlaspack_plugin_rpc/src/plugin/resolver.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/atlaspack_plugin_rpc/src/plugin/resolver.rs b/crates/atlaspack_plugin_rpc/src/plugin/resolver.rs index e0d2f8980..b3e7dade6 100644 --- a/crates/atlaspack_plugin_rpc/src/plugin/resolver.rs +++ b/crates/atlaspack_plugin_rpc/src/plugin/resolver.rs @@ -1,11 +1,11 @@ -use std::fmt; -use std::fmt::Debug; - +use anyhow::anyhow; use atlaspack_config::PluginNode; use atlaspack_core::plugin::PluginContext; use atlaspack_core::plugin::ResolveContext; use atlaspack_core::plugin::Resolved; use atlaspack_core::plugin::ResolverPlugin; +use std::fmt; +use std::fmt::Debug; #[derive(Hash)] pub struct RpcResolverPlugin {} @@ -24,6 +24,6 @@ impl RpcResolverPlugin { impl ResolverPlugin for RpcResolverPlugin { fn resolve(&self, _ctx: ResolveContext) -> Result { - todo!() + Err(anyhow!("Not implemented")) } } From d4468f49a7d1d6689569399f4133563e64818aec Mon Sep 17 00:00:00 2001 From: Pedro Yamada Date: Tue, 17 Sep 2024 07:36:01 +0000 Subject: [PATCH 04/10] Force the asset request to run on the rust version Since we do not have cache invalidation properly set-up with the rust implementation. It's better to force it for now. This should, if our implementation is correct fix certain tests that depend on cache invalidation. --- packages/core/integration-tests/test/api.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/integration-tests/test/api.js b/packages/core/integration-tests/test/api.js index d4cb3bdd1..4965d6164 100644 --- a/packages/core/integration-tests/test/api.js +++ b/packages/core/integration-tests/test/api.js @@ -14,8 +14,8 @@ import { import {ATLASPACK_VERSION} from '../../core/src/constants'; -describe.v2('JS API', function () { - it('should respect distEntry', async function () { +describe('JS API', function () { + it.v2('should respect distEntry', async function () { const NAME = 'custom-name.js'; let b = await bundle( From 0da341d851d3eb3cd5fb1285fd05ba083f420d9d Mon Sep 17 00:00:00 2001 From: Pedro Yamada Date: Tue, 17 Sep 2024 07:36:23 +0000 Subject: [PATCH 05/10] Force the asset request to run on the rust version Since we do not have cache invalidation properly set-up with the rust implementation. It's better to force it for now. This should, if our implementation is correct fix certain tests that depend on cache invalidation. --- packages/core/core/src/requests/BundleGraphRequest.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/core/core/src/requests/BundleGraphRequest.js b/packages/core/core/src/requests/BundleGraphRequest.js index bb9130d44..762582faa 100644 --- a/packages/core/core/src/requests/BundleGraphRequest.js +++ b/packages/core/core/src/requests/BundleGraphRequest.js @@ -114,7 +114,9 @@ export default function createBundleGraphRequest( let {assetGraph, changedAssets, assetRequests} = await api.runRequest( request, { - force: options.shouldBuildLazily && requestedAssetIds.size > 0, + force: + Boolean(input.rustAtlaspack) || + (options.shouldBuildLazily && requestedAssetIds.size > 0), }, ); From 219944b1ea7a805b00ebb2a0ef0a94862d7a698f Mon Sep 17 00:00:00 2001 From: Pedro Yamada Date: Tue, 17 Sep 2024 07:37:52 +0000 Subject: [PATCH 06/10] Enable integration tests that are green on rust version --- .../integration-tests/test/BundleGraph.js | 4 +- .../core/integration-tests/test/bundler.js | 568 +++++++++--------- .../integration-tests/test/contentHashing.js | 51 +- 3 files changed, 323 insertions(+), 300 deletions(-) diff --git a/packages/core/integration-tests/test/BundleGraph.js b/packages/core/integration-tests/test/BundleGraph.js index 474f624f8..5755da660 100644 --- a/packages/core/integration-tests/test/BundleGraph.js +++ b/packages/core/integration-tests/test/BundleGraph.js @@ -11,7 +11,7 @@ import { } from '@atlaspack/test-utils'; import type {BundleGraph, BundleGroup, PackagedBundle} from '@atlaspack/types'; -describe.v2('BundleGraph', () => { +describe('BundleGraph', () => { it('can traverse assets across bundles and contexts', async () => { let b = await bundle( path.join(__dirname, '/integration/worker-shared/index.js'), @@ -85,7 +85,7 @@ describe.v2('BundleGraph', () => { ]); }); - describe('getBundlesInBundleGroup', () => { + describe.v2('getBundlesInBundleGroup', () => { let bundleGraph: BundleGraph; let bundleGroup: BundleGroup; let dir = path.join(__dirname, 'get-bundles-in-bundle-group'); diff --git a/packages/core/integration-tests/test/bundler.js b/packages/core/integration-tests/test/bundler.js index 39266bfd3..9056ae4d3 100644 --- a/packages/core/integration-tests/test/bundler.js +++ b/packages/core/integration-tests/test/bundler.js @@ -6,15 +6,15 @@ import { bundle, describe, findAsset, + fsFixture, it, overlayFS, - fsFixture, run, } from '@atlaspack/test-utils'; import {hashString} from '@atlaspack/rust'; import {normalizePath} from '@atlaspack/utils'; -describe.v2('bundler', function () { +describe('bundler', function () { it('should not create shared bundles when a bundle is being reused and disableSharedBundles is enabled', async function () { await fsFixture(overlayFS, __dirname)` disable-shared-bundle-single-source @@ -532,8 +532,10 @@ describe.v2('bundler', function () { ]); }); - it('should not count inline assests towards parallel request limit', async function () { - await fsFixture(overlayFS, __dirname)` + it.v2( + 'should not count inline assests towards parallel request limit', + async function () { + await fsFixture(overlayFS, __dirname)` inlined-assests buzz.js: export default 7; @@ -562,291 +564,309 @@ describe.v2('bundler', function () { yarn.lock:`; - // Shared bundle should not be removed in this case - let b = await bundle(path.join(__dirname, 'inlined-assests/local.html'), { - mode: 'production', - defaultTargetOptions: { - shouldScopeHoist: false, - }, - inputFS: overlayFS, - }); - - assertBundles(b, [ - { - assets: ['local.html'], - }, - { - assets: ['buzz.js'], - }, - { - assets: [ - 'inline-module.js', - 'local.html', - 'bundle-url.js', - 'cacheLoader.js', - 'js-loader.js', - ], - }, - { - assets: ['esmodule-helpers.js'], - }, - ]); - }); - - it('should not create a shared bundle from an asset if that asset is shared by less than minBundles bundles', async function () { - let b = await bundle( - path.join(__dirname, 'integration/min-bundles/index.js'), - { + // Shared bundle should not be removed in this case + let b = await bundle(path.join(__dirname, 'inlined-assests/local.html'), { mode: 'production', defaultTargetOptions: { shouldScopeHoist: false, }, - }, - ); + inputFS: overlayFS, + }); - assertBundles(b, [ - { - name: 'index.js', - assets: [ - 'index.js', - 'bundle-url.js', - 'cacheLoader.js', - 'css-loader.js', - 'esmodule-helpers.js', - 'js-loader.js', - 'bundle-manifest.js', - ], - }, - { - // a and b are shared between only 2 bundles so they are kept in each bundle - assets: ['bar.js', 'a.js', 'b.js'], - }, - { - assets: ['buzz.js'], - }, - { - assets: ['a.js', 'b.js', 'foo.js'], - }, - { - // c is shared between 3 different bundles, so it stays - assets: ['c.js'], - }, - { - assets: ['styles.css'], - }, - { - assets: ['local.html'], - }, - ]); - }); + assertBundles(b, [ + { + assets: ['local.html'], + }, + { + assets: ['buzz.js'], + }, + { + assets: [ + 'inline-module.js', + 'local.html', + 'bundle-url.js', + 'cacheLoader.js', + 'js-loader.js', + ], + }, + { + assets: ['esmodule-helpers.js'], + }, + ]); + }, + ); + + it.v2( + 'should not create a shared bundle from an asset if that asset is shared by less than minBundles bundles', + async function () { + let b = await bundle( + path.join(__dirname, 'integration/min-bundles/index.js'), + { + mode: 'production', + defaultTargetOptions: { + shouldScopeHoist: false, + }, + }, + ); - it('should remove reused bundle (over shared bundles based on size) if the bundlegroup hit the parallel request limit', async function () { - let b = await bundle( - path.join( - __dirname, - 'integration/shared-bundle-reused-bundle-remove-reuse/index.js', - ), - { - mode: 'production', - defaultTargetOptions: { - shouldScopeHoist: false, + assertBundles(b, [ + { + name: 'index.js', + assets: [ + 'index.js', + 'bundle-url.js', + 'cacheLoader.js', + 'css-loader.js', + 'esmodule-helpers.js', + 'js-loader.js', + 'bundle-manifest.js', + ], }, - }, - ); + { + // a and b are shared between only 2 bundles so they are kept in each bundle + assets: ['bar.js', 'a.js', 'b.js'], + }, + { + assets: ['buzz.js'], + }, + { + assets: ['a.js', 'b.js', 'foo.js'], + }, + { + // c is shared between 3 different bundles, so it stays + assets: ['c.js'], + }, + { + assets: ['styles.css'], + }, + { + assets: ['local.html'], + }, + ]); + }, + ); + + it.v2( + 'should remove reused bundle (over shared bundles based on size) if the bundlegroup hit the parallel request limit', + async function () { + let b = await bundle( + path.join( + __dirname, + 'integration/shared-bundle-reused-bundle-remove-reuse/index.js', + ), + { + mode: 'production', + defaultTargetOptions: { + shouldScopeHoist: false, + }, + }, + ); - assertBundles(b, [ - { - name: 'index.js', - assets: [ - 'index.js', - 'bundle-url.js', - 'cacheLoader.js', - 'css-loader.js', - 'esmodule-helpers.js', - 'js-loader.js', - 'bundle-manifest.js', - ], - }, - { - assets: ['bar.js', 'foo.js', 'a.js', 'b.js'], - }, - { - assets: ['buzz.js'], - }, - { - assets: ['c.js'], - }, - { - assets: ['a.js', 'b.js', 'foo.js'], - }, - { - assets: ['styles.css'], - }, - { - assets: ['local.html'], - }, - ]); - }); + assertBundles(b, [ + { + name: 'index.js', + assets: [ + 'index.js', + 'bundle-url.js', + 'cacheLoader.js', + 'css-loader.js', + 'esmodule-helpers.js', + 'js-loader.js', + 'bundle-manifest.js', + ], + }, + { + assets: ['bar.js', 'foo.js', 'a.js', 'b.js'], + }, + { + assets: ['buzz.js'], + }, + { + assets: ['c.js'], + }, + { + assets: ['a.js', 'b.js', 'foo.js'], + }, + { + assets: ['styles.css'], + }, + { + assets: ['local.html'], + }, + ]); + }, + ); //This test case is the same as previous except we remove the shared bundle since it is smaller - it('should remove shared bundle (over reused bundles based on size) if the bundlegroup hit the parallel request limit', async function () { - let b = await bundle( - path.join( - __dirname, - 'integration/shared-bundle-reused-bundle-remove-shared/index.js', - ), - { - mode: 'production', - defaultTargetOptions: { - shouldScopeHoist: false, + it.v2( + 'should remove shared bundle (over reused bundles based on size) if the bundlegroup hit the parallel request limit', + async function () { + let b = await bundle( + path.join( + __dirname, + 'integration/shared-bundle-reused-bundle-remove-shared/index.js', + ), + { + mode: 'production', + defaultTargetOptions: { + shouldScopeHoist: false, + }, }, - }, - ); - - assertBundles(b, [ - { - name: 'index.js', - assets: [ - 'index.js', - 'bundle-url.js', - 'cacheLoader.js', - 'css-loader.js', - 'esmodule-helpers.js', - 'js-loader.js', - 'bundle-manifest.js', - ], - }, - { - assets: ['bar.js', 'c.js'], - }, - { - // A consequence of our shared bundle 'c' being removed for the bundleGroup bar - // is that it must also be removed for buzz, even though the buzz bundleGroup does not - // hit the parallel request limit. This is because the shared bundle is no longer sharing - // it is only attached to one bundle and thus should be removed. - assets: ['buzz.js', 'c.js'], - }, - { - assets: ['a.js', 'b.js', 'foo.js'], - }, - { - assets: ['styles.css'], - }, - { - assets: ['local.html'], - }, - ]); - }); + ); - it('should not remove shared bundle from graph if one bundlegroup hits the parallel request limit, and at least 2 other bundleGroups that need it do not', async function () { - //The shared bundle should only be 'put back' for the bundlegroups which hit the parallel request limit - // But if there are at least two other bundlegroups using this shared bundle that do not hit the max limit - // the shared bundle should not be removed from the graph - let b = await bundle( - path.join( - __dirname, - 'integration/shared-bundle-remove-from-one-group-only/index.js', - ), - { - mode: 'production', - defaultTargetOptions: { - shouldScopeHoist: false, + assertBundles(b, [ + { + name: 'index.js', + assets: [ + 'index.js', + 'bundle-url.js', + 'cacheLoader.js', + 'css-loader.js', + 'esmodule-helpers.js', + 'js-loader.js', + 'bundle-manifest.js', + ], }, - }, - ); - - assertBundles(b, [ - { - name: 'index.js', - assets: [ - 'index.js', - 'bundle-url.js', - 'cacheLoader.js', - 'css-loader.js', - 'esmodule-helpers.js', - 'js-loader.js', - 'bundle-manifest.js', - ], - }, - { - assets: ['bar.js', 'c.js'], // shared bundle merged back - }, - { - assets: ['buzz.js'], - }, - { - assets: ['c.js'], // shared bundle - }, - { - assets: ['foo.js'], - }, - { - assets: ['styles.css'], - }, - { - assets: ['local.html'], - }, - ]); - }); + { + assets: ['bar.js', 'c.js'], + }, + { + // A consequence of our shared bundle 'c' being removed for the bundleGroup bar + // is that it must also be removed for buzz, even though the buzz bundleGroup does not + // hit the parallel request limit. This is because the shared bundle is no longer sharing + // it is only attached to one bundle and thus should be removed. + assets: ['buzz.js', 'c.js'], + }, + { + assets: ['a.js', 'b.js', 'foo.js'], + }, + { + assets: ['styles.css'], + }, + { + assets: ['local.html'], + }, + ]); + }, + ); + + it.v2( + 'should not remove shared bundle from graph if one bundlegroup hits the parallel request limit, and at least 2 other bundleGroups that need it do not', + async function () { + //The shared bundle should only be 'put back' for the bundlegroups which hit the parallel request limit + // But if there are at least two other bundlegroups using this shared bundle that do not hit the max limit + // the shared bundle should not be removed from the graph + let b = await bundle( + path.join( + __dirname, + 'integration/shared-bundle-remove-from-one-group-only/index.js', + ), + { + mode: 'production', + defaultTargetOptions: { + shouldScopeHoist: false, + }, + }, + ); - it('should not remove shared bundle from graph if its parent (a reused bundle) is removed by parallel request limit', async function () { - //The shared bundle should only be 'put back' for the bundlegroups which hit the parallel request limit - // But if there are at least two other bundlegroups using this shared bundle that do not hit the max limit - // the shared bundle should not be removed from the graph - let b = await bundle( - path.join( - __dirname, - 'integration/shared-bundle-between-reused-bundle-removal/index.js', - ), - { - mode: 'production', - defaultTargetOptions: { - shouldScopeHoist: false, + assertBundles(b, [ + { + name: 'index.js', + assets: [ + 'index.js', + 'bundle-url.js', + 'cacheLoader.js', + 'css-loader.js', + 'esmodule-helpers.js', + 'js-loader.js', + 'bundle-manifest.js', + ], }, - }, - ); + { + assets: ['bar.js', 'c.js'], // shared bundle merged back + }, + { + assets: ['buzz.js'], + }, + { + assets: ['c.js'], // shared bundle + }, + { + assets: ['foo.js'], + }, + { + assets: ['styles.css'], + }, + { + assets: ['local.html'], + }, + ]); + }, + ); + + it.v2( + 'should not remove shared bundle from graph if its parent (a reused bundle) is removed by parallel request limit', + async function () { + //The shared bundle should only be 'put back' for the bundlegroups which hit the parallel request limit + // But if there are at least two other bundlegroups using this shared bundle that do not hit the max limit + // the shared bundle should not be removed from the graph + let b = await bundle( + path.join( + __dirname, + 'integration/shared-bundle-between-reused-bundle-removal/index.js', + ), + { + mode: 'production', + defaultTargetOptions: { + shouldScopeHoist: false, + }, + }, + ); - assertBundles(b, [ - { - name: 'index.js', - assets: [ - 'index.js', - 'bundle-url.js', - 'cacheLoader.js', - 'css-loader.js', - 'esmodule-helpers.js', - 'js-loader.js', - 'bundle-manifest.js', - ], - }, - { - assets: ['bar.js', 'foo.js', 'a.js', 'b.js'], // shared bundle merged back - }, - { - assets: ['buzz.js'], - }, - { - assets: ['c.js'], // shared bundle - }, - { - assets: ['foo.js', 'a.js', 'b.js'], - }, - { - assets: ['styles.css'], - }, - { - assets: ['local.html'], - }, - ]); + assertBundles(b, [ + { + name: 'index.js', + assets: [ + 'index.js', + 'bundle-url.js', + 'cacheLoader.js', + 'css-loader.js', + 'esmodule-helpers.js', + 'js-loader.js', + 'bundle-manifest.js', + ], + }, + { + assets: ['bar.js', 'foo.js', 'a.js', 'b.js'], // shared bundle merged back + }, + { + assets: ['buzz.js'], + }, + { + assets: ['c.js'], // shared bundle + }, + { + assets: ['foo.js', 'a.js', 'b.js'], + }, + { + assets: ['styles.css'], + }, + { + assets: ['local.html'], + }, + ]); - assert( - b - .getReferencedBundles(b.getBundlesWithAsset(findAsset(b, 'bar.js'))[0]) - .includes(b.getBundlesWithAsset(findAsset(b, 'c.js'))[0]), - ); - }); + assert( + b + .getReferencedBundles( + b.getBundlesWithAsset(findAsset(b, 'bar.js'))[0], + ) + .includes(b.getBundlesWithAsset(findAsset(b, 'c.js'))[0]), + ); + }, + ); - it('should split manifest bundle', async function () { + it.v2('should split manifest bundle', async function () { let b = await bundle( [ path.join(__dirname, 'integration/split-manifest-bundle/a.html'), @@ -1019,7 +1039,7 @@ describe.v2('bundler', function () { ]); }); - it('should support inline constants', async () => { + it.v2('should support inline constants', async () => { await fsFixture(overlayFS, __dirname)` inline-constants-shared-bundles one.html: @@ -1093,7 +1113,7 @@ describe.v2('bundler', function () { ]); }); - it('should support inline constants with shared bundles', async () => { + it.v2('should support inline constants with shared bundles', async () => { await fsFixture(overlayFS, __dirname)` inline-constants-shared-bundles one.html: @@ -1258,7 +1278,7 @@ describe.v2('bundler', function () { 'async value should not be inlined', ); }); - describe('manual shared bundles', () => { + describe.v2('manual shared bundles', () => { const dir = path.join(__dirname, 'manual-bundle'); beforeEach(() => { diff --git a/packages/core/integration-tests/test/contentHashing.js b/packages/core/integration-tests/test/contentHashing.js index d97dcafa0..f3b931858 100644 --- a/packages/core/integration-tests/test/contentHashing.js +++ b/packages/core/integration-tests/test/contentHashing.js @@ -21,12 +21,12 @@ function bundle(path) { }); } -describe.v2('content hashing', function () { +describe('content hashing', function () { beforeEach(async () => { await outputFS.rimraf(path.join(__dirname, '/input')); }); - it('should update content hash when content changes', async function () { + it.v2('should update content hash when content changes', async function () { await ncp( path.join(__dirname, '/integration/html-css'), path.join(__dirname, '/input'), @@ -59,7 +59,7 @@ describe.v2('content hashing', function () { assert.notEqual(filename, newFilename); }); - it('should update content hash when raw asset changes', async function () { + it.v2('should update content hash when raw asset changes', async function () { await ncp( path.join(__dirname, '/integration/import-raw'), path.join(__dirname, '/input'), @@ -94,25 +94,28 @@ describe.v2('content hashing', function () { ); }); - it('should generate the same hash for the same distDir inside separate projects', async () => { - let a = await _bundle( - path.join(__dirname, 'integration/hash-distDir/a/index.html'), - {sourceMaps: true}, - ); - let b = await _bundle( - path.join(__dirname, 'integration/hash-distDir/b/index.html'), - {sourceMaps: true}, - ); - - let aBundles = a.getBundles(); - let bBundles = b.getBundles(); - - assert.equal(aBundles.length, 2); - assert.equal(bBundles.length, 2); - - let aJS = aBundles.find(bundle => bundle.type === 'js'); - let bJS = bBundles.find(bundle => bundle.type === 'js'); - assert(/index\.[a-f0-9]*\.js/.test(path.basename(aJS.filePath))); - assert.equal(aJS.name, bJS.name); - }); + it.v2( + 'should generate the same hash for the same distDir inside separate projects', + async () => { + let a = await _bundle( + path.join(__dirname, 'integration/hash-distDir/a/index.html'), + {sourceMaps: true}, + ); + let b = await _bundle( + path.join(__dirname, 'integration/hash-distDir/b/index.html'), + {sourceMaps: true}, + ); + + let aBundles = a.getBundles(); + let bBundles = b.getBundles(); + + assert.equal(aBundles.length, 2); + assert.equal(bBundles.length, 2); + + let aJS = aBundles.find(bundle => bundle.type === 'js'); + let bJS = bBundles.find(bundle => bundle.type === 'js'); + assert(/index\.[a-f0-9]*\.js/.test(path.basename(aJS.filePath))); + assert.equal(aJS.name, bJS.name); + }, + ); }); From 075d601944f3c0c15fd3edda637344a51cfa4a2f Mon Sep 17 00:00:00 2001 From: Pedro Yamada Date: Tue, 17 Sep 2024 07:50:23 +0000 Subject: [PATCH 07/10] Fix test utils linter error --- packages/core/test-utils/src/utils.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/core/test-utils/src/utils.js b/packages/core/test-utils/src/utils.js index e8da59ca1..6e89bc23d 100644 --- a/packages/core/test-utils/src/utils.js +++ b/packages/core/test-utils/src/utils.js @@ -1338,9 +1338,11 @@ function itImpl( try { await builder.call(this); } catch (err) { + // eslint-disable-next-line no-console console.error(`TEST: ${title} failed for V3`, err); return; } + // eslint-disable-next-line no-console console.error(`TEST: ${title} is green for V3 but skipped`); }, args, From ab58d8751a6714f86a6238d2132532d80703d30b Mon Sep 17 00:00:00 2001 From: Pedro Yamada Date: Wed, 18 Sep 2024 00:45:29 +0000 Subject: [PATCH 08/10] Fix todo message hang on integration tests --- crates/atlaspack_plugin_resolver/src/atlaspack_resolver.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/crates/atlaspack_plugin_resolver/src/atlaspack_resolver.rs b/crates/atlaspack_plugin_resolver/src/atlaspack_resolver.rs index edc66a694..d2ef99488 100644 --- a/crates/atlaspack_plugin_resolver/src/atlaspack_resolver.rs +++ b/crates/atlaspack_plugin_resolver/src/atlaspack_resolver.rs @@ -361,7 +361,9 @@ impl ResolverPlugin for AtlaspackResolver { (atlaspack_resolver::Resolution::External, _query) => { if let Some(_source_path) = &ctx.dependency.source_path { if ctx.dependency.env.is_library && ctx.dependency.specifier_type != SpecifierType::Url { - todo!("check excluded dependency for libraries"); + return Err(anyhow::anyhow!( + "Not implemented: We need to check excluded dependency for libraries" + )); } } From dcb5149791d265278f63c0485dd5d221a9b21396 Mon Sep 17 00:00:00 2001 From: Pedro Yamada Date: Wed, 18 Sep 2024 01:15:33 +0000 Subject: [PATCH 09/10] Lower timeouts and self-timeout --- packages/core/test-utils/src/utils.js | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/packages/core/test-utils/src/utils.js b/packages/core/test-utils/src/utils.js index 6e89bc23d..168249657 100644 --- a/packages/core/test-utils/src/utils.js +++ b/packages/core/test-utils/src/utils.js @@ -1335,8 +1335,20 @@ function itImpl( this, title, async function () { + // Timeout mocha after 3s when running V2 tests over V3, but also self + // timeout after 2s to avoid hanging the test suite. + this.timeout(3000); + this.retries(0); + try { - await builder.call(this); + await Promise.race([ + builder.call(this), + new Promise((_, reject) => { + setTimeout(() => { + reject(new Error('TEST: Timeout')); + }, 2000); + }), + ]); } catch (err) { // eslint-disable-next-line no-console console.error(`TEST: ${title} failed for V3`, err); From f861158ba3e61957aa3c7f999bb7af9357857f59 Mon Sep 17 00:00:00 2001 From: Pedro Yamada Date: Thu, 19 Sep 2024 00:01:13 +0000 Subject: [PATCH 10/10] Minor patches to running the tests --- .../integration-tests/test/BundleGraph.js | 24 +++++++++++-------- packages/core/test-utils/src/utils.js | 8 +++---- 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/packages/core/integration-tests/test/BundleGraph.js b/packages/core/integration-tests/test/BundleGraph.js index 5755da660..f3b67b54a 100644 --- a/packages/core/integration-tests/test/BundleGraph.js +++ b/packages/core/integration-tests/test/BundleGraph.js @@ -86,11 +86,12 @@ describe('BundleGraph', () => { }); describe.v2('getBundlesInBundleGroup', () => { - let bundleGraph: BundleGraph; - let bundleGroup: BundleGroup; let dir = path.join(__dirname, 'get-bundles-in-bundle-group'); - before(async () => { + async function setupTest(): Promise<{ + bundleGraph: BundleGraph, + bundleGroup: BundleGroup, + }> { await overlayFS.mkdirp(dir); await fsFixture(overlayFS, dir)` @@ -103,20 +104,21 @@ describe('BundleGraph', () => { yarn.lock: {} `; - bundleGraph = await bundle(path.join(dir, 'index.jsx'), { + const bundleGraph = await bundle(path.join(dir, 'index.jsx'), { inputFS: overlayFS, }); - - bundleGroup = bundleGraph.getBundleGroupsContainingBundle( + const bundleGroup = bundleGraph.getBundleGroupsContainingBundle( bundleGraph.getBundles({includeInline: true})[0], )[0]; - }); + return {bundleGraph, bundleGroup}; + } after(async () => { await overlayFS.rimraf(dir); }); - it('does not return inlineAssets by default', () => { + it('does not return inlineAssets by default', async () => { + const {bundleGraph, bundleGroup} = await setupTest(); const bundles = bundleGraph.getBundlesInBundleGroup(bundleGroup); assert.deepEqual( @@ -125,7 +127,8 @@ describe('BundleGraph', () => { ); }); - it('does not return inlineAssets when requested', () => { + it('does not return inlineAssets when requested', async () => { + const {bundleGraph, bundleGroup} = await setupTest(); const bundles = bundleGraph.getBundlesInBundleGroup(bundleGroup, { includeInline: false, }); @@ -136,7 +139,8 @@ describe('BundleGraph', () => { ); }); - it('returns inlineAssets when requested', () => { + it('returns inlineAssets when requested', async () => { + const {bundleGraph, bundleGroup} = await setupTest(); const bundles = bundleGraph.getBundlesInBundleGroup(bundleGroup, { includeInline: true, }); diff --git a/packages/core/test-utils/src/utils.js b/packages/core/test-utils/src/utils.js index 168249657..4e02014b5 100644 --- a/packages/core/test-utils/src/utils.js +++ b/packages/core/test-utils/src/utils.js @@ -1335,9 +1335,9 @@ function itImpl( this, title, async function () { - // Timeout mocha after 3s when running V2 tests over V3, but also self - // timeout after 2s to avoid hanging the test suite. - this.timeout(3000); + // Timeout mocha after 10s when running V2 tests over V3, but also self + // timeout after 9s to avoid hanging the test suite. + this.timeout(10000); this.retries(0); try { @@ -1346,7 +1346,7 @@ function itImpl( new Promise((_, reject) => { setTimeout(() => { reject(new Error('TEST: Timeout')); - }, 2000); + }, 9000); }), ]); } catch (err) {