Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

In spec tests, use expectTemplate over equals and shouldThrow #1683

Merged
merged 11 commits into from
May 4, 2020

Conversation

jbboehr
Copy link
Contributor

@jbboehr jbboehr commented Apr 24, 2020

So, I hate to change a bunch of code for relatively little benefit, but I have a project that exports the specification tests as JSON [0] and, well, it would make my life a lot easier if expectTemplate was used instead of equals or shouldThrow in the test suite, where possible.

I would also convert shouldCompileTo if you'd like, but I figured I'd keep the changes to a minimum for now.

I added two methods, withHelpers and withPartials to HandlebarsTestBench only because it let me port a bunch of tests with fewer changes. If you'd prefer, I would remove those and convert the tests to use withHelper and withPartial directly.

I tried to keep the changes generally linear so it would be easier to review. Let me know if there's anything you'd like changed.

@jbboehr
Copy link
Contributor Author

jbboehr commented Apr 24, 2020

Looks like HandlebarsTestBench as-is ignores it's message property. Maybe this?

diff --git a/spec/env/common.js b/spec/env/common.js
index 5997322..1a78619 100644
--- a/spec/env/common.js
+++ b/spec/env/common.js
@@ -183,7 +183,7 @@ HandlebarsTestBench.prototype.withMessage = function(message) {
 };
 
 HandlebarsTestBench.prototype.toCompileTo = function(expectedOutputAsString) {
-  expect(this._compileAndExecute()).to.equal(expectedOutputAsString);
+  expect(this._compileAndExecute()).to.equal(expectedOutputAsString, this.message);
 };
 
 // see chai "to.throw" (https://www.chaijs.com/api/bdd/#method_throw)

@nknapp
Copy link
Collaborator

nknapp commented Apr 26, 2020

First of all, I wouldn't say that this change has little benefit. I have often looked at test cases and had a hard time to understand what was actually tested, especially with the shouldCompileTo-method.
That was the reason for me to introduce the expectTemplate-method in the first place.

I think replacing all calls (also to shouldCompileTo) is a valuable task and I am glad that you volunteer to do it. I thought I even had created an issue for it, but I cannot find it anymore...

Of course, the thing with tests is, that if you want to ensure backwards compatibility, you shouldn't change them. But on the other hand, it will make the code much more readable. The important thing is that they are really semantically equal to the old version.

I haven't had a detailed look at your PR yet, but please proceed with replacing shouldCompileTo as well.

@nknapp
Copy link
Collaborator

nknapp commented Apr 26, 2020

Looks like HandlebarsTestBench as-is ignores it's message property. Maybe this?

diff --git a/spec/env/common.js b/spec/env/common.js
index 5997322..1a78619 100644
--- a/spec/env/common.js
+++ b/spec/env/common.js
@@ -183,7 +183,7 @@ HandlebarsTestBench.prototype.withMessage = function(message) {
 };
 
 HandlebarsTestBench.prototype.toCompileTo = function(expectedOutputAsString) {
-  expect(this._compileAndExecute()).to.equal(expectedOutputAsString);
+  expect(this._compileAndExecute()).to.equal(expectedOutputAsString, this.message);
 };
 
 // see chai "to.throw" (https://www.chaijs.com/api/bdd/#method_throw)

Yes, that's right. I don't know why I didn't use it. Maybe do this as a separate commit so that it can be more easily reverted. I would even discard the third parameter of toThrow in favor of using this.message

@nknapp
Copy link
Collaborator

nknapp commented Apr 26, 2020

I should probably also share my vision about the spec and about language test-cases:

My long-term goal which also addresses #1277, is to write a specification text, but also include test-cases in yaml form (independent of any language). An example is this file, but this is just an early thought. It would be cool to have some kind of IDE support to write those examples, but not strictly necessary.

Helpers are, of course, difficult to describe that way, but this can probably be solved similar to the lambdas in the mustache-spec.

I want to mention this, because rewriting all tests using expectTemplate, and your goal of generating JSON, is a first step into this direction and most of the tests should somehow be part of the spec.

I should probably start setting up another VuePress site containing the language spec.

Copy link
Collaborator

@nknapp nknapp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, that was a lot of work. Sadly also a lot of work to review... Before merging, I want to have a look at each test, but today, I only came as far as the end of basic.js. I have added some comments and change requests.

Those are not things that you have done wrong, but rather things that can be improved further, now that you replaced everything by expectTemplate. I'll add another review later on, but for now, I just want my review saved...

spec/basic.js Show resolved Hide resolved
spec/basic.js Show resolved Hide resolved
spec/basic.js Outdated Show resolved Hide resolved
spec/basic.js Outdated Show resolved Hide resolved
spec/basic.js Outdated Show resolved Hide resolved
spec/basic.js Outdated Show resolved Hide resolved
@jbboehr
Copy link
Contributor Author

jbboehr commented Apr 26, 2020

First of all, I wouldn't say that this change has little benefit.

Well I just felt a little bad waltzing in here and changing the git-blame on the entire test suite, that's all :)

Of course, the thing with tests is, that if you want to ensure backwards compatibility, you shouldn't change them.

Should be pretty safe. I could try to write some code that verified the inputs to CompilerContext were binary-equal, but that seems like overkill.

Maybe do this as a separate commit so that it can be more easily reverted. I would even discard the third parameter of toThrow in favor of using this.message

Done.

My long-term goal [...] is to write a specification text, but also include test-cases in yaml form (independent of any language)

That would be great! I'll have to stick to my Frankenstein creation in [0] in the meantime, of course.

I hope you'll also throw up a page with a list of all the different implementations at some point, if there isn't already. They can be hard to track down sometimes.

Helpers are, of course, difficult to describe that way, but this can probably be solved similar to the lambdas in the mustache-spec.

Feel free to ping me when you need a bunch of javascript converted to PHP. For C, I have to resort to this abomination [2].


A quick overview of my recent commits. In the interest of avoiding transcription errors, I wrote a converter [1] to convert shouldCompileTo and shouldCompileToWithPartials to use expectTemplate.

There are currently individual commits for:

  • the original large batch of equals and shouldThrow I ported by hand to use expectTemplate (wasn't really possible to do any automatic conversion)
  • the discussed changes to message in the test bench
  • two different test cases my converter had trouble with I ported by hand
  • adding withDecorator and withDecorators to the test bench
  • one big commit with the automatic conversion

Of course, feel free to request any changes, rebases, rewords, squashes, merges, etc. I'm going to try and do another manual review of the whole thing, but it'll probably take a while.

@jbboehr
Copy link
Contributor Author

jbboehr commented Apr 26, 2020

@nknapp Sorry there wasn't any context, typing up my previous post took a while, I should have prepared it beforehand.

Sadly also a lot of work to review

Yeah, I was a little worried doing most of the work in advance that the review burden would be too high to have it merged :S

@jbboehr
Copy link
Contributor Author

jbboehr commented Apr 26, 2020

Perhaps off-topic, but going over the partial tests and there is this one test:

  it('partials with undefined context', function() {
    var string = 'Dudes: {{>dude dudes}}';
    var partial = '{{foo}} Empty';
    var hash = {};
    shouldCompileToWithPartials(
      string,
      [hash, {}, { dude: partial }],
      true,
      'Dudes:  Empty'
    );
  });

but the context is {} not undefined. I will patch it in my branch (it's passing).

EDIT: I'm just dumb it was referring to the partial context, not the root context.

Copy link
Collaborator

@nknapp nknapp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, I didn't make it the whole way through. I found some places where variables should be inlined. But altogether I like it very much.

Kudos to you, that's a lot of changes that you committed there. I'll continue my review during the next days (hopefully).

spec/builtins.js Outdated Show resolved Hide resolved
spec/builtins.js Outdated Show resolved Hide resolved
spec/builtins.js Show resolved Hide resolved
spec/builtins.js Outdated Show resolved Hide resolved
spec/builtins.js Outdated Show resolved Hide resolved
@nknapp
Copy link
Collaborator

nknapp commented Apr 29, 2020

Apart from the review, I have been wondering about these lines: https://github.com/handlebars-lang/handlebars.js/blob/master/spec/builtins.js#L228-L243

It would be really cool to get completely remove "compileWithPartials", but this call cannot be replaced easier: I am thinking about adding a new method to expectTemplate:

  • Either .toCompileToAnyOf(expectedResult1, expectedResult2, ...)
  • or something that can be called like .toCompileToMatching(result => result === expectedResult1 || result === expectedResult2)

Do you have any preference in this? The first one is more explicite, and since you are not using JavaScript, if would probably be more useful to you. The second one is more flexible, in case other use cases come along.

I think I would tend to use .toCompileToAnyOf

@jbboehr
Copy link
Contributor Author

jbboehr commented Apr 29, 2020

@nknapp I would lean towards the former. If you wanted something more generic, using a regex would allow a bit more flexibility while still being used easily from other languages.

@jbboehr
Copy link
Contributor Author

jbboehr commented Apr 29, 2020

Looks like the build failed due to a timeout in tasks/task-tests/git.test.js.

Copy link
Collaborator

@nknapp nknapp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reviewed two more files.
I really didn't realize up-front how much work this is. I think next time it would be better to do things like this in multiple PRs with changing only some files.

spec/data.js Outdated Show resolved Hide resolved
spec/data.js Outdated Show resolved Hide resolved
spec/data.js Show resolved Hide resolved
@nknapp
Copy link
Collaborator

nknapp commented Apr 30, 2020

@nknapp I would lean towards the former. If you wanted something more generic, using a regex would allow a bit more flexibility while still being used easily from other languages.

Good. I will see if I change it, but rather in a separate PR.

@nknapp
Copy link
Collaborator

nknapp commented Apr 30, 2020

Looks like the build failed due to a timeout in tasks/task-tests/git.test.js.

I'll see to that in the end, when I have reviewed all files. Maybe it helps to just increase the timeout.

jbboehr added a commit to jbboehr/handlebars-spec that referenced this pull request May 1, 2020
Currently using using my fork: https://github.com/jbboehr/handlebars.js/tree/remove-equals-4.x
Awaiting PR: handlebars-lang/handlebars.js#1683

* Rewrote everything in TypeScript
* Switched the license to `AGPL-3.0-or-later`. The specification data is still licensed under the `MIT` license, as it is extracted from `handlebars.js`
* `description` now includes all `describe($description, ...)` from the handlebars test suite
* `exception` can now be either `true`, a string, or a regex.
* `message` used to be the exception message, but will now be any extra message noted in the handlebars.js test suite
* `options` was renamed to `runtimeOptions` to differentiate from `compileOptions`
* Using a new version format `104.7.6` which is: `(myMajor * 100 + handlebarsMajor) + '.' + (myMinor * 100 + handlebarsMinor) + '.' + (myPatch * 100 + handlebarsPatch)`
* `number` is now included in tests (besides the first implied `00`) that have multiple cases
* `compat` is removed in favor of `compileOptions.compat`
* `globalPartials`, `globalDecorators`, and `globalHelpers` are now removed and merged into
  `partials`, `decorators`, and `helpers` instead

Closes #7
Copy link
Collaborator

@nknapp nknapp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still have to review 4 files. I have added a small comment, but this is something that could have been better before. You don't have to fix it. I can do that later as well (but I might forget about it, so I wanted to take a note).

CompilerContext.compileWithPartial = CompilerContext.compile;
expectTemplate('{{> dude}}')
.withPartials({ dude: 'fail' })
.toThrow(Error, /The partial dude could not be compiled/);
handlebarsEnv.compile = compile;
Copy link
Collaborator

@nknapp nknapp May 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resetting the functions would be better in a finally-block.

This was referenced Mar 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants