🐛 AppVeyor test HTML: should support bundling HTML fails

[DeMoorJasper]

🐛 bug report
On AppVeyor i can see this test is lately failing a lot.

1) html should support bundling HTML:
      AssertionError [ERR_ASSERTION]: 3 == 2
      + expected - actual
      -3
      +2
      
      at assertBundleTree (test\utils.js:117:12)
      at Context.<anonymous> (test\html.js:9:5)
      at <anonymous>
      at process._tickCallback (internal/process/next_tick.js:188:7)
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
Command exited with code 1

🎛 Configuration (.babelrc, package.json, cli command)

AppVeyor config of this Repo

🤔 Expected Behavior

Should pass

😯 Current Behavior

Doesn't pass

[pomle]

Hello @DeMoorJasper!
Why was this closed?

I have this problem and seems to be an indeterministic test.

Here's some output from ~10 runs of while true; do yarn test -- --grep 'should support bundling HTML'; done

About half fail.

https://gist.github.com/pomle/283c6f16d0b4e4e5e4947ad58fce1abb#file-parcel-indeterministic-html-test-txt

Also seen problems with watcher should only re-package bundles that changed which looks like a drifting timestamp issue.

1) watcher should only re-package bundles that changed:

      AssertionError [ERR_ASSERTION]: [ 1513331454, 1513331454 ] deepEqual [ 1513331454, 1513331455 ]
      + expected - actual

       [
         1513331454
      -  1513331454
      +  1513331455
       ]

      at Context.<anonymous> (test/watcher.js:122:12)
      at <anonymous>

[DeMoorJasper]

This was a mistake indeed, (didn’t accure the day i closed it)

_{Sent with GitHawk}

[pomle]

I've looked into this but need more information.

In fc8ce69#diff-08e28e98a118d512b13f4fc7423da77aR668 the HTML test is introduced, and the integration fixtures looks looks like

html
├── index.css
├── index.html
├── index.js
└── other.html

Both index.html and other.html points to both index.css and index.js, but the comparison fixture for the bundler looks like

{
  name: 'index.html',
  assets: ['index.html'],
  childBundles: [
    {
      type: 'css',
      assets: ['index.css'],
      childBundles: []
    },
    {
      type: 'html',
      assets: ['other.html'],
      childBundles: [
        {
          type: 'js',
          assets: ['index.js'],
          childBundles: []
        }
      ]
    }
  ]
}

Describing a tree where index.html has children index.css, other.html, and other.html has index.js as only child.

This may be correct behavior. However, given this integration fixture structure, the let b = await bundle(__dirname + '/integration/html/index.html') call will create indeterministic bundler trees according to my experimentation.

I have found the following discrepant behaviors to occur frequently (>50% of test runs).

index.html having three children.

1) html should support bundling HTML:

      AssertionError [ERR_ASSERTION]: 3 == 2
      + expected - actual

      -3
      +2

      at assertBundleTree (test/utils.js:117:12)
      at Context.<anonymous> (test/html.js:9:5)
      at <anonymous>
      at process._tickCallback (internal/process/next_tick.js:188:7)

index.html having one child.
html

    1) should support bundling HTML


  0 passing (3s)
  1 failing

  1) html should support bundling HTML:

      AssertionError [ERR_ASSERTION]: 1 == 2
      + expected - actual

      -1
      +2

      at assertBundleTree (test/utils.js:119:12)
      at Context.<anonymous> (test/html.js:9:5)
      at <anonymous>

js bundle being of css type.

  html
bundle.name /mnt/c/Users/pom/Development/pom/parcel/test/dist/index.html tree.name index.html
bundle.name /mnt/c/Users/pom/Development/pom/parcel/test/dist/3dd25c636124a624507fecf2d5196e35.js tree.name undefined
    1) should support bundling HTML


  0 passing (3s)
  1 failing

  1) html should support bundling HTML:

      AssertionError [ERR_ASSERTION]: 'js' == 'css'
      + expected - actual

      -js
      +css

      at assertBundleTree (test/utils.js:99:12)
      at tree.childBundles.forEach (test/utils.js:120:41)
      at Array.forEach (<anonymous>)
      at assertBundleTree (test/utils.js:120:23)
      at Context.<anonymous> (test/html.js:9:5)
      at <anonymous>

Without me knowing too much about the internals I can't draw any conclusions now.

I believe @devongovett is the original author of this test so should be able to shine some light on it.

[brandon93s]

I've identified this issue. We'll need to decide:

• Is this actually an issue, or are we OK with nondeterministic? (improve test)
• If we fix it, there is several ways we can do so. Let's discuss options. (improve code)

Explanation:

The following block of code is nondeterministic, and is causing the test failures:

parcel/src/Bundler.js

Lines 326 to 340 in c008bb0

	await Promise.all(
	dependencies.map(async dep => {
	let assetDep = await this.resolveDep(asset, dep);
	if (dep.includedInParent) {
	// This dependency is already included in the parent's generated output,
	// so no need to load it. We map the name back to the parent asset so
	// that changing it triggers a recompile of the parent.
	this.loadedAssets.set(dep.name, asset);
	} else {
	asset.dependencies.set(dep.name, dep);
	asset.depAssets.set(dep.name, assetDep);
	await this.loadAsset(assetDep);
	}
	})
	);

The above code spins up a set of promises, each of which alters the same Map(s) as they complete. Since these are promises, they can complete in any order, and therefore interact with the Map(s) in any order. The most problematic of these being asset.dependencies.set(dep.name, dep);.

When the test passes, asset.dependencies contains ./index.css, ./other.html, ./index.js in that order. When the test fails, asset.dependencies contains ./index.css, ./index.js, ./other.html in that order.

Since a Map iterates in insertion order, when the dependencies are later iterated through to generate the bundle tree, inconsistent ordering results in different trees.

parcel/src/Bundler.js

Lines 393 to 396 in c008bb0

	for (let dep of asset.dependencies.values()) {
	let assetDep = asset.depAssets.get(dep.name);
	this.createBundleTree(assetDep, dep, bundle);
	}

[brandon93s]

Reproduction Steps:

You can impose an artificial delay in order to consistently reproduce this locally:

dependencies.map(async dep => { 
   if (~dep.name.indexOf('other')) await new Promise(r => setTimeout(r, 500)); // add
   let assetDep = await this.resolveDep(asset, dep); 

This should result in the test in question failing.

[devongovett]

Should be fixed by 539ade1