FWIW, I ran npm run test on this branch, and then git diff-index --name-only HEAD --.

The tests all passed as expected, and the git diff-index command turned up clean.

Hmm, I think this change is a little confusing and changes what is being tested.

In the original test:

{
    binInputParameters: [
      '-i',
      '<div>1</div>',
      '-a',
      '-m',
      '-N',
      'test',
      '--map',
      './spec/artifacts/source.map.amd.txt'
    ],
    outputLocation: 'stdout',
    expectedOutputSpec: './spec/expected/source.map.amd.js'
  },

./spec/artifacts/source.map.amd.txt

Contains the sourcemap:

{"version":3,"sources":["test"],"names":["compiler","main","container","depth0","helpers","partials","data","useData"],"mappings":"kHAAA,CAAAA,SAAA,CAAA,EAAA,YAAAC,KAAA,SAAAC,EAAAC,EAAAC,EAAAC,EAAAC,GAAA,MAAA,gBAAAC,SAAA"}

And spec/expected/source.map.amd.js contains a compiled handlebars template with a sourcemap ref:

define(["handlebars.runtime"],function(e){var t=(e=e.default).template;return(e.templates=e.templates||{}).test=t({compiler:[8,">= 4.3.0"],main:function(e,t,a,n,i){return"<div>1</div>"},useData:!0})});
//# sourceMappingURL=./spec/artifacts/source.map.amd.txt

With the new test:

 {
    binInputParameters: [
      '-i',
      '<div>1</div>',
      '-a',
      '-m',
      '-N',
      'test',
      '--map',
      './spec/artifacts/source.map.amd.json'
    ],
    outputLocation: './spec/artifacts/source.map.amd.json',
    expectedOutputSpec: './spec/expected/source.map.amd.json'
  },

We're now testing the sourcemap has the correct config, not the template.

That might well be a valid test, but it's a different test to the one that was there before 🙈

Nothing to do with this PR, outputLocation declares where the test should look for its output, rather than changing where the output goes, which I also find a bit confusing.

We're now testing the sourcemap has the correct config, not the template.

This is a bi-product of passing a string via cli to the the handlebars precompiler vs. reading from a template file.

We're now testing the sourcemap has the correct config, not the template.

IMO, this is a better unit test than the previous one as it asserts the content of the source map. We can improve on this by adding another assertion to test that the source map is correctly referenced within the compiled template. This will require us to switch from passing a string to the complier via the cli to reading from a template file.

Let me know your thoughts. Happy to close the gap as per the above comments.

IMO, this is a better unit test than the previous one as it asserts the content of the source map.

I disagree 🙂 Handlebars isn't responsible for the content of the source map being correct, that's the job of the source-map module.

The original test checked that the handlebars template had the correct sourcemap string, which is the behaviour specific to handlebars.

A second thing to test would be that the linked source map file actually exists, but I see no reason to verify the contents.

TBH I think the correct fix for this issue is Nils' suggestion.

I've used the tmp dir approach to fix this locally and am going to push it to unblock the release.

If you would like to update this PR to add an additional test to check that the sourcemap file is created, and make the other modifications here, will merge after release :)

I disagree 🙂 Handlebars isn't responsible for the content of the source map being correct, that's the job of the source-map module.

Quiet right; I'm convinced :)

I'll update the tests accordingly.

Do we still need this PR or can it be closed?

Let's keep this open as until I've had a chance to review what we landed in 15.x as an alternative to this change.

I hope to get it this weekend.

@aorinevo Is this still relevant?