search

ocurrent / current-bench

Experimental benchmarking infrastructure using OCurrent pipelines
Apache License 2.0
33 stars 17 forks source link

Improve json identification with FSA #365

Closed ElectreAAS closed 2 years ago

ElectreAAS commented 2 years ago

This mainly resolves #363 I only added a small test to pinpoint the problem, I'll work on putting more tests in there in this PR

art-w commented 2 years ago

Very cool! Thanks for adding more tests, the automata is quite involved :) Can we also have an example of "wrong json followed by valid json" to check that it recovers? (also json anywhere in a line since you also fixed that!)

art-w commented 2 years ago

Thanks!

punchagan commented 2 years ago

@ElectreAAS I tried testing this with the local-test-repo/Makefile and it seems to error out. Could you take a look?

You could trigger a build using this Makefile by running make local-make-bench, for instance.

ElectreAAS commented 2 years ago

@punchagan I don't want to be that person, but... it works on my machine :laughing: if I cd local-test-repo; make bench as advised by @art-w, it seems to work...? It prints a lot of things, but looking at the Makefile it doesn't do anything, just calls to echo. What errors do you have?

If at the root I make local-make-bench it will try to git commit stuff, but will stop since there are no changes... Very weird makefiles

ElectreAAS commented 2 years ago

By the way the only thing make local-make-bench does is this: cd local-test-repo/; git add .; git commit --amend -m "New commit: $$(date)"; cd .., which is... not very useful?

art-w commented 2 years ago

:D So due to the lack of a good computer, you are not currently running current-bench which is why you don't see the issue! The local-test-repo is used as a test repository that current-bench monitors during development: On a change (typically triggered by the useless make local-make-bench), current-bench will automatically re-run the make bench command from local-test-repo: The output of that command contains the json metrics that will be parsed and stored in the database.

So what @punchagan is saying is that the updated json parser fails when trying to parse the json output of the test repository, which isn't a good sign :)

punchagan commented 2 years ago

Thanks for adding more context to my comment, @art-w !