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

HTML report missing jquery code #127

Closed
erikd opened this issue Nov 15, 2016 · 18 comments
Closed

HTML report missing jquery code #127

erikd opened this issue Nov 15, 2016 · 18 comments

Comments

@erikd
Copy link
Member

erikd commented Nov 15, 2016

I have two very similar machines (both x86_64, running Debian Testing) and I build the same project on the two machines, with a constraint criterion == 1.1.1.0 (see #126).

When I run the benchmark on both machines, one produces a properly working HTML report and the other produces a HTML page with nothing but a blank line where the jquery/javascript should be.

I can't figure out what is different on the two machines.

erikd added a commit to erikd/fastpack that referenced this issue Nov 15, 2016
@erikd
Copy link
Member Author

erikd commented Nov 15, 2016

The ghc and cabal-install programs on the two machines are installed from the same binary packages.

@RyanGlScott
Copy link
Member

Hm, can you upload the HTML report that has improperly working JavaScript on it? I've been noticing similar things (with criterion-1.1.4.0, mind you, but I don't suspect the issue affects multiple versions of criterion) when I benchmark code that runs for a particularly long time, resulting in something like #65. A symptom of this is xxx entries in the report.

But it's also possible that this is something else entirely—I'd need to take a closer look to be sure.

@erikd
Copy link
Member Author

erikd commented Nov 15, 2016

Bad one is here: http://www.mega-nerd.com/erikd/bench-bad.html

@RyanGlScott
Copy link
Member

As I feared, this is #65 once again showing up, as indicated by all the xxx entries. (If you examine logs as it's running, you'll probably see lots of NaN values). So now we have reports of it happening on both criterion-1.1.1.0 and criterion-1.1.4.0.

FWIW, on my project (text-show), this kind of error occurs non-deterministically once every 10 runs or so, and usually happens about half-way through the individual benchmarks. That is, the first 5 plots or so will work, but then one run will somehow conjure up a NaN value, causing that plot (and all subsequent plots) to be rendered incorrectly in HTML/JavaScript.

Do you get this error often when you run it? If so, and you're willing to share the code, it might help me diagnose the issue faster, since this is much closer to a Heisenbug on my machine/project.

@erikd
Copy link
Member Author

erikd commented Nov 15, 2016

Code is at https://github.com/erikd/fastpack .

On some machines I get this every run, on others only occasionally.

@erikd
Copy link
Member Author

erikd commented Nov 15, 2016

Ok, this seems to be related with whether --no-gc is added to the command line when I run the benchmark.

@erikd
Copy link
Member Author

erikd commented Nov 16, 2016

Bah, figured it out and nothing to do with --no-gc.

I sometimes use a custom template and the terms in the default templates that are replaced had changed.

     <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
     <title>criterion report</title>
     <script language="javascript" type="text/javascript">
-      {{#include}}js/jquery-2.1.1.min.js{{/include}}
+      {{js-jquery}}
     </script>
     <script language="javascript" type="text/javascript">
-      {{#include}}js/jquery.flot-0.8.3.min.js{{/include}}
+      {{js-flot}}
     </script>
     <script language="javascript" type="text/javascript">
       {{#include}}js/jquery.criterion.js{{/include}}

Would be nice if there was an actual error or warning printed when the template replacement failed.

@RyanGlScott
Copy link
Member

I must admit that I'm terribly confused as to how this pertains to the reported issue. Firstly, are you still talking about your custom template producing HTML that lacks jQuery code, or did you mean to comment on #126 instead (your phrase "template replacement failed" in #127 (comment) leads me to think possibly the latter).

I'd also like a clarification on how to reproduce this. I noticed in fastpack that you call this from your Makefile:

dist/build/bench/bench --no-gc -o bench.html --template=Criterion/report.tpl

Is the --template bit here necessary to reproduce the bug?

@erikd
Copy link
Member Author

erikd commented Nov 16, 2016

Seems I should avoid writing bug reports late in the evenings and early in the mornings :/. Seems I managed to confuse myself as well as you.

Yes, --template is necessary.

@RyanGlScott
Copy link
Member

Yes, --template is necessary.

Aha! If I go to commit a75e9ff3b055737194cc08694e4036c30fe3956e of fastpack and run make bench.html, then I get an HTML report that looks identical to the one here.

Back to the original question of error messages: I'm not sure if there's much that criterion can do from its end. It delegates the role of template substitution to hastacheStr from the hastache library (as found here in criterion). AFAICT, hastache doesn't throw any kind of warnings whatsoever upon a template substitution failure, which is unfortunate.

I think if we wanted better warnings, we'd probably need to switch from hastache to another mustache templating library. See #119.

@erikd
Copy link
Member Author

erikd commented Nov 16, 2016

I think if we wanted better warnings, we'd probably need to switch from hastache to another mustache templating library. See #119.

Agreed!

@RyanGlScott
Copy link
Member

criterion-1.2.0.0 now uses microstache instead of hastache. However, the situation regarding error messages when template substitution fails isn't much better, since there still aren't any error messages! I think the culprits are these lines of microstache:

lookupKey :: Key -> Render Value
lookupKey (Key []) = NE.head <$> asks rcContext
lookupKey k = do
  v     <- asks rcContext
  p     <- asks rcPrefix
  pname <- asks (templateActual . rcTemplate)
  let f x = asum (simpleLookup False (x <> k) <$> v)
  case asum (fmap (f . Key) . reverse . tails $ unKey p) of
    -- Context Misses: Failed context lookups should be considered falsey.
    -- throw (MustacheRenderException pname (p <> k))
    Nothing -> return (String "")
    Just  r -> return r

Notice that the line which would normally throw an exception is commented out. @phadej, is there a reason for doing this? The stache library itself does throw an exception, and this behavior was changed in haskellari/microstache@d152ca6#diff-de3d2f0d810c141fa05451f9fd97a9a9L195.

@phadej
Copy link
Contributor

phadej commented May 24, 2017

@RyanGlScott it's to adhere to mustache spec. It treats non-existing keys as falsey:

https://github.com/mustache/spec/blob/83b0721610a4e11832e83df19c73ace3289972b9/specs/sections.yml#L135-L139

Not sure, if the same lookupKey is used for {{var}} and {{#section}} though. I could check if someone opens an issue (I have no idea what kind of template code and substitution should throw exception in this case).

@phadej
Copy link
Contributor

phadej commented May 24, 2017

amendment to the previous: "silent missing variables" are per spec: https://github.com/mustache/spec/blob/83b0721610a4e11832e83df19c73ace3289972b9/specs/interpolation.yml#L106-L110

So it's mustache to blame, I'm open to suggestions though. We can introduce an option(s) to make interpolations more strict. What mustache libraries in other languages do?

@RyanGlScott
Copy link
Member

The mustache library, for instance, has a checkedSubstitute which also returns errors. It doesn't halt upon errors, as the spec requires that (as you noted), but if microstache had something like this, we could at least produce warnings when something goes awry during substitution.

@phadej
Copy link
Contributor

phadej commented May 24, 2017

that's a nice idea; the rendered monad is already a state, so adding the warnings shouldn't be too difficult. Could you open issue, i already switched the context; so might take some time until I get to it.

@RyanGlScott
Copy link
Member

I've opened haskellari/microstache#3 for this feature request.

@RyanGlScott
Copy link
Member

See #152 for an attempt to make warnings more explicit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants