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

Add unit tests to client target #15

Open
brunogouveia opened this issue May 13, 2018 · 5 comments
Open

Add unit tests to client target #15

brunogouveia opened this issue May 13, 2018 · 5 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@brunogouveia
Copy link
Contributor

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).

@loganek loganek added enhancement New feature or request good first issue Good for newcomers labels May 16, 2018
@nkhlktn
Copy link

nkhlktn commented Oct 18, 2020

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

@loganek
Copy link
Member

loganek commented Oct 18, 2020

@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.

@nkhlktn
Copy link

nkhlktn commented Oct 22, 2020

@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.

@loganek
Copy link
Member

loganek commented Oct 25, 2020

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.

@nkhlktn
Copy link

nkhlktn commented Nov 1, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants