[PATCH][LNT] Add ability to filter machines on daily report page + refactor LNT regression tests to enable properly testing it.

Kristof Beyls kristof.beyls at arm.com
Mon Jun 8 04:40:38 PDT 2015


Thanks for the quick review James.

Some comments inline below:

> From: James Molloy [mailto:james at jamesmolloy.co.uk] 
> Sent: 08 June 2015 10:41
> To: Kristof Beyls; Chris Matthews
> Cc: llvm-commits
> Subject: Re: [PATCH][LNT] Add ability to filter machines on daily report page + refactor LNT regression tests to enable properly testing it.
>
> 0001:
>
> +                              is not None]
> You don't need is not None here. .search() will evaluate to a Match object or None, which can be treated as a boolean.

Indeed - my preference for explicitness shows up here. I was in doubt on writing the explicit "is not None", and you
picked on it, so I'll remove it before committing.

> +              }}" rel="stylesheet" media="screen"/>
>
> Is there any particular reason you've XHTML'd up these tags? It looks like there's a lot of HTML cleanup in this patch that probably shouldn't be there. Stuff like adding </tr> I don't think anyone can argue with (just commit that?) but adding /> to the end of tags isn't HTML, it's XHTML and is not normally done, especially for img,link and meta tags.

The reason I did this is to be able to easily parse the page using python's built-in XML parser in the unit tests I've added.
I looked into python's builtin HTML parsing library. It doesn't create a tree representation of the document, it only calls
event handlers when encountering tags. I think having a tree representation available is more handy when writing unit tests.
Python's default XML parser does create such a tree representation, which is why I went for using that one, and fixing
incorrect syntax and changing to XHTML in a few places.
If there would be a good builtin HTML parser creating tree representations in python, I'd be happy to stick to HTML instead of
XHTML, but I don't think there is a such a parser in the python default libraries.

>
> Otherwise this looks fine to me, and thanks for the tests!

> 0002: 
> +  {{ utils.regex_filter_box('filter', '.searchable tr', "Machines...") }}
>
> It looks like you've moved this around the page - from before the <h3>Machines</h3> to after it. Was this deliberate?

Yes, it is deliberate, as I think the box is in a more logical place when moved. But obviously, I shouldn't call this
a non-functional change, which I claim in the commit message. I'll move the moving of the box to a separate commit.

> +    (function ($) {
>
> I'm not sure why this nested function is necessary, but it was just copy-paste so I won't mention any more about it.
>
> 0003 and 0004: Instead of trying to work out the column to search on, I think it would be easier if you would just give all cells that are searchable some extra class, such as "td-searchable". Then, you wouldn't need to parameterize the build_regex macro; it could just always do something like "$(this).find('td.td-searchable').text()". How does that sound? Especially in 0004, this might simplify things.

If done that way, a mechanism will need to be found to figure out from the selected "td" that matches, which is the DOM element that needs
to be hidden (so far those are all <tr>s containing the <td>). One way or another, there needs to be a link between the DOM element that needs
to be hidden/shown, and which part of it that needs to be searched for the regex. As long as this functionality will only hide/show table
rows and search table cells, the alternative you propose may end up being a bit simpler. The way it's implemented now, it's probably a bit
more generic. I guess that the best solution depends on whether in the future we might want to do regex searches to hide/show stuff
other than table rows and cells.








More information about the llvm-commits mailing list