mozilla / active-data-recipes

A repository of various activedata queries and recipes
Mozilla Public License 2.0
8 stars 24 forks source link

Recipe to track skipped tests #69

Closed MadinaB closed 5 years ago

MadinaB commented 5 years ago

Could you please review following to check whether I am moving to right direction? This is related to issue#27

MadinaB commented 5 years ago

Hello, @ahal!

Thank you for your explanations. Now it is more clear which kind of data should be returned. Could you please check it for now?

screen shot 2018-11-03 at 11 41 07 pm

(Due to long returned values it does not seem neat in default table format)

screen shot 2018-11-03 at 11 41 27 pm
ahal commented 5 years ago

Thanks, looks better!

Some notes:

  1. You can just display the "fullname" of the suite rather than the whole config object
  2. Looks like there's 1 really long test name that's messing up the table formatting. But that is a separate issue from this..
  3. This is way too much data to be useful, so I think we need to limit it somehow. I see few options: a) Require a platform to be specified on the command line (e.g linux64/opt) b) Alternatively, require a suite on the command line and display platforms in the data c) Restrict the data to the "mozilla-central" branch (actually we should make sure to do this anyway)

Anyway, this is getting close to a point where I'd be comfortable merging it and then improving iteratively.

MadinaB commented 5 years ago

Thank you for the review! I will update that

MadinaB commented 5 years ago

There was a typo in "today-day" according to here it should be at least "today-1day", but still it did not return any results. As well as, "today-3day" did not return any results, but "today-10day" started to return something. However, now I see that "today-1day" is still incorrect, we should get data from "today" to "eod" in ideal. But still nothing is returned with this update:

screen shot 2018-11-07 at 12 04 33 am
MadinaB commented 5 years ago

However, I do not think this is problem of code since in this time interval it still does not return anything with yyyy-mm-dd format:

screen shot 2018-11-07 at 12 25 36 am

But in the last month, it returns:

screen shot 2018-11-07 at 12 25 28 am
MadinaB commented 5 years ago

Setting repo.branch.name to "mozilla-central" did not shorten results for a lot:

screen shot 2018-11-07 at 12 06 31 am

But specifying test suite actually did that well:

screen shot 2018-11-07 at 12 07 08 am

If no run suite is specified, it gets data from all suites.

MadinaB commented 5 years ago

Platform option is provided, as well, but it is not required one:

screen shot 2018-11-07 at 12 07 39 am screen shot 2018-11-07 at 12 08 38 am

So if needed platform can be specified, but if not data from all platforms is being taken.

ahal commented 5 years ago

Setting repo.branch.name to "mozilla-central" did not shorten results for a lot:

Yeah, I wouldn't expect this to reduce the number of tests specificially, but it should greatly reduce the counts. So while the end table might not look much smaller, the amount of data being processed should be. The count field isn't actually that important (it basically just says how many times the test ran). We mostly want to know whether the test was skipped in the most recent data we have.

ahal commented 5 years ago

However, I do not think this is problem of code since in this time interval it still does not return anything with yyyy-mm-dd format:

Yeah I suspect your code is fine. Sometimes ActiveData lags a bit behind the live data. Let's just leave this alone for now.

ahal commented 5 years ago
* That all suites' names are hard coded and I do not like this moment. In [code_coverage_by_suite.py](https://github.com/mozilla/active-data-recipes/blob/master/adr/recipes/code_coverage_by_suite.py) all suites are also hard coded, but I expect that if test suites are going to change, this data will not be actual any more. One solution could be create new query for getting all suites present in ActiveData.

I agree that I don't like the fact that suites are hard-coded. I also like your solution, having a simple query like that would be a good building block for many different recipes! However in this case I think you could achieve the same thing by not doing any filtering by suite. The problem here is that we would either need two separate queries (one that has the suite filter and one that doesn't) or we'd need to build the query dynamically (depending on whether or not a suite was passed in). This is a problem that ADR could try to solve more generally but I'm not really sure how.

I'll leave it up to you what you prefer (either use two separate queries, or create a new query that returns the list of suites).

MadinaB commented 5 years ago

Thanks ^_^

I am glad that you liked the idea, I am also viewing that this could be more elegant way to solve this. I will take a look on how to improve it. Dynamic query generation looks as really attractive option, but yet more complicated one. I will try in either way. :)

MadinaB commented 5 years ago

Hi, @ahal!

Hope you are doing well!

I am sorry for late reply.

I was excited about solving this part, but finally it got me :)

This commit works.. I did not expect this one to work before I checked. All because argparse does take missing optional arguments as None, jsone.render() updates all arguments fitting this None values into query and json.dump(query) updates all None values to null. When ActiveData faces something like this with null value, it treats absolutely differently from what was expected ... T_T

"where":{"and":[{"eq":{"run.suite.fullname":null}}]}

I expected it will get only those values where "run.suite.fullname" is equal to null or will return nothing if there are no such values... However, it treats that as if null would aggregate all values except null and since we do provide nothing except null, it aggregates all the values of run.suite.fullname. Like it was really driving me nuts why this was happening

With {"eq":{"run.suite.fullname":null}}:

screen shot 2018-11-11 at 12 25 20 am

Without {"eq":{"run.suite.fullname":null}}:

screen shot 2018-11-11 at 12 25 26 am

With {"eq":{"run.suite.fullname":null}}:

screen shot 2018-11-11 at 12 25 49 am

Without {"eq":{"run.suite.fullname":null}}:

screen shot 2018-11-11 at 12 25 54 am

I was looking whole around the docs and even run greps across ActiveData code, but figured out that that is ActiveData itself treating nulls differently..

However, it is still equal to the commit aab3346 since when we assign to filter something and pass null as something, ActiveData makes null to cover everything and we filter by checking whether it matches everything and it is like we do not need any filtering at all.

This one works, but before I figured it out I did PR#82, which does update query dynamically according to filters provided.

ahal commented 5 years ago

Ok, good investigating! We could confirm with ekyle, but I guess that ActiveData behaves this way for exactly this reason! (So we don't have to write separate queries to handle null values)

MadinaB commented 5 years ago

Thank you!

Sorting by path added as you suggested!

Here is before:

screen shot 2018-11-14 at 2 35 51 am

Here is after:

screen shot 2018-11-14 at 2 36 18 am
ahal commented 5 years ago

This is amazing, thank you so much!

I have some ideas for improving this if you'd like to keep working on this recipe. But I can also understand if you'd like to focus on something else for a bit.

MadinaB commented 5 years ago

Thank you @ahal,

Yes, I would like to continue working on this and would be glad to get any new directions.