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

writeout: support to generate JSON output #4870

Closed
wants to merge 3 commits into from

Conversation

@mgumz
Copy link
Contributor

mgumz commented Feb 1, 2020

This commit adds support to generate JSON via the writeout feature:

-w "%{json}"

It leverages the existing infrastructure as much as possible. Thus,
generating the JSON on STDERR is possible by:

-w "%{stderr}%{json}"

This implements a variant of https://github.com/curl/curl/wiki/JSON#--write-out-json.

Copy link
Contributor

emilengler left a comment

I like the idea especially because JSON is nowadays the preferred way to parse data

src/tool_writeout_json.c Outdated Show resolved Hide resolved
src/tool_writeout_json.c Outdated Show resolved Hide resolved
src/tool_writeout_json.c Outdated Show resolved Hide resolved
src/tool_writeout_json.c Outdated Show resolved Hide resolved
@emilengler

This comment has been minimized.

Copy link
Contributor

emilengler commented Feb 1, 2020

The Linter is failing btw.
Also I used this feature on https://emilengler.com and got a malicious result

"time_total":0,127754

The comma should be a point to be valid json. Also I personally would prefer to format the JSON human readable (with indents and newlines) but this is more of a non blocking issue

@emilengler

This comment has been minimized.

Copy link
Contributor

emilengler commented Feb 1, 2020

Nevermind, looks like that's because of my shell

@mgumz

This comment has been minimized.

Copy link
Contributor Author

mgumz commented Feb 1, 2020

yes, the "," originates from your environment BUT .. the feature should be a bit more robust coz the goal is to produce valid JSON.

its different for the regular "-w" output coz there you get "text".

@mgumz mgumz force-pushed the mgumz:feature/writeout-json branch 4 times, most recently from 846bfe8 to c7a786c Feb 1, 2020
src/tool_writeout_json.c Outdated Show resolved Hide resolved
docs/cmdline-opts/write-out.d Show resolved Hide resolved
@bagder

This comment has been minimized.

Copy link
Member

bagder commented Feb 3, 2020

We would also need a test or two added that verify that this actually does it is supposed to do!

@emilengler

This comment has been minimized.

Copy link
Contributor

emilengler commented Feb 3, 2020

@mgumz @bagder
Would it be ok if I would write the test?
Will create the PR based on this branch.

@bagder

This comment has been minimized.

Copy link
Member

bagder commented Feb 3, 2020

I would welcome it, but the decision is really @mgumz's to make...

@emilengler

This comment has been minimized.

Copy link
Contributor

emilengler commented Feb 3, 2020

I'm not sure if I'll master it to write a test. I don't know how to verify dynamic variables such as time_total

@mgumz

This comment has been minimized.

Copy link
Contributor Author

mgumz commented Feb 4, 2020

@mgumz @bagder
Would it be ok if I would write the test?
Will create the PR based on this branch.

sure!

@bagder

This comment has been minimized.

Copy link
Member

bagder commented Feb 4, 2020

I don't know how to verify dynamic variables such as time_total

Right, they are tricky. One way to test those things is to:

  1. make those tests only run in debug-builds
  2. have the code #ifdefed on debug check the presence of an env variable
  3. if the env variable is set, use that as value instead of the "actual"
  4. Like for example CURL_TIME = 3.1415

We have a bunch of tests working like this already.

@mgumz

This comment has been minimized.

Copy link
Contributor Author

mgumz commented Feb 4, 2020

I don't know how to verify dynamic variables such as time_total

Right, they are tricky. One way to test those things is to:

  1. make those tests only run in debug-builds
  2. have the code #ifdefed on debug check the presence of an env variable
  3. if the env variable is set, use that as value instead of the "actual"
  4. Like for example CURL_TIME = 3.1415

We have a bunch of tests working like this already.

Or if there is a CURL struct which is actually filled with pseudo data and this struct is fed into the writeoutJSON function … then the only thing required is to actually provide a *FILE which ends up in a string which is then compared against the expected value.

src/tool_writeout_json.c Outdated Show resolved Hide resolved
src/tool_writeout_json.c Outdated Show resolved Hide resolved
@bagder

This comment has been minimized.

Copy link
Member

bagder commented Feb 8, 2020

I've created an initial take at a test case for this functionality, sitting in my writeout-json-test branch.

@mgumz mgumz force-pushed the mgumz:feature/writeout-json branch 7 times, most recently from ad105b4 to 3112a01 Feb 13, 2020
@bagder

This comment has been minimized.

Copy link
Member

bagder commented Feb 17, 2020

The red CI jobs seem to be unrelated.

@bagder

This comment has been minimized.

Copy link
Member

bagder commented Feb 18, 2020

The reason the test 955 fails is because I made it return '13' to all the values that can change, and with your update it now means 13 microseconds while it previously meant 13 seconds, so the expected output in the test needs to be adjusted accordingly!

@mgumz

This comment has been minimized.

Copy link
Contributor Author

mgumz commented Feb 18, 2020

can i launch a specific test per command?

@mgumz

This comment has been minimized.

Copy link
Contributor Author

mgumz commented Feb 18, 2020

can i launch a specific test per command?

yes: perl runtests.pl 955

@mgumz mgumz force-pushed the mgumz:feature/writeout-json branch 2 times, most recently from 566225a to 6ab8311 Feb 18, 2020
@mgumz

This comment has been minimized.

Copy link
Contributor Author

mgumz commented Feb 18, 2020

k, i also tweaked curl_version() (similar to CURL_GETHOSTNAME, CURL_TIME) to get a stable version string which can be used to test against in the unit-tests.

@mgumz

This comment has been minimized.

Copy link
Contributor Author

mgumz commented Feb 20, 2020

@bagder ping ... any more ... whishes / hints / suggestions / improvements?

@bagder

This comment has been minimized.

Copy link
Member

bagder commented Feb 27, 2020

I think it looks fine. We won't merge this until after the pending release as this is a new feature, not a bugfix.

It also now needs a rebase (and changed test number since 955 was taken).

Maybe spend the time and fix so that all the write-out strings aren't duplicated in the new file? Having them duplicated like this will only cause them to get out of sync and generally be hard to maintain.

@mgumz mgumz force-pushed the mgumz:feature/writeout-json branch 2 times, most recently from befb898 to 2b6d0d9 Feb 29, 2020
@mgumz

This comment has been minimized.

Copy link
Contributor Author

mgumz commented Mar 1, 2020

rebased to current master + put a new commit ontop which is tackles the duplication of the fields in tool_writeout* … could be some sort of approach to tackle some pieces of the existing tool_writeout.c parts as well.

tool_writeout_json.h is kind of useless now, tool_writeout.c could #include "tool_writeout_json.c" directly … but as a first discussion i think the new commit is worth having.

@mgumz mgumz force-pushed the mgumz:feature/writeout-json branch from 2b6d0d9 to f6587ab Mar 4, 2020
mgumz and others added 3 commits Feb 1, 2020
This commit adds support to generate JSON via the writeout feature:

    -w "%{json}"

It leverages the existing infrastructure as much as possible. Thus,
generating the JSON on STDERR is possible by:

    -w "%{stderr}%{json}"

This implements a variant of https://github.com/curl/curl/wiki/JSON#--write-out-json.
Makes curl_easy_getinfo() of "variable" numerical content instead return
the number set in the env variable `CURL_TIME`.

Makes curl_version() of "variable" textual content. This guarantees a
stable version string which can be tested against. Environment variable
`CURL_VERSION` defines the content.
With the introduction of tool_writeout_json.c the list of fields
to write out is significant.

This commit refactors some data structures to keep the source of
truth to one place only.
@mgumz mgumz force-pushed the mgumz:feature/writeout-json branch from f6587ab to 0928457 Mar 7, 2020
@mgumz

This comment has been minimized.

Copy link
Contributor Author

mgumz commented Mar 11, 2020

ping @bagder

@bagder bagder self-assigned this Mar 17, 2020
@bagder

This comment has been minimized.

Copy link
Member

bagder commented Mar 17, 2020

Thanks!

@bagder bagder closed this in 04c0341 Mar 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.