hawktracer icon indicating copy to clipboard operation
hawktracer copied to clipboard

Add unit tests to client target

Open brunogouveia opened this issue 6 years ago • 5 comments

Currently we don't have unit tests for the client (aka, hawktracer-to-json).

We should add unit tests, specially to avoid regression in fixes we have done already (e.g, issue #2).

brunogouveia avatar May 13 '18 16:05 brunogouveia

Is it fair to say that the current test coverage is limited to CallGraph in the client? I was thinking of picking this up.

nkhlktn avatar Oct 18 '20 11:10 nkhlktn

@nikhilkotian22 I think it's fair to slightly de-scope this ticket and say it's really about covering ChromeTraceConverter with tests. I'm glad you consider picking this up, please let me know if you need any further assistance.

loganek avatar Oct 18 '20 13:10 loganek

@loganek

  • Do you have an example of the expected events input to the trace output? Let's start with a known data initially and keep increasing later on?

  • Do you think adding further abstractions would make sense at this point of the project? Lets says for example the ChromeTraceConverter is a specialisation as well as the std::ofstream.

nkhlktn avatar Oct 22 '20 19:10 nkhlktn

Do you have an example of the expected events input to the trace output? Let's start with a known data initially and keep increasing later on?

@nikhilkotian22 I think for unit-testing purposes, it's best to create instaces of Event and define classes depending on your tests. We don't have any pre-defined test events, however, you might want to check tests for parser to see how to construct events.

Do you think adding further abstractions would make sense at this point of the project? Lets says for example the ChromeTraceConverter is a specialisation as well as the std::ofstream.

I think it'd be good idea to pass std::ostream object as parameter to init function instead of filename so we could in the future pass other than regular file outputs (e.g. cout or custom implementation of ostream). However, we don't have any usecase for that at the moment, so I wouldn't say it's very needed at this point of time.

loganek avatar Oct 25 '20 12:10 loganek

@loganek understood. I have made progress with tests. Will upload a PR soon. Some corner cases left.

nkhlktn avatar Nov 01 '20 15:11 nkhlktn