ICRAR / daliuge

The DALiuGE Execution Engine
GNU Lesser General Public License v2.1
24 stars 7 forks source link

Validate graphs #212

Closed james-strauss-uwa closed 1 year ago

james-strauss-uwa commented 2 years ago

Added a CI task to validate logical graphs in the repository against the logical graph JSON schema. The CI task fails if any graphs are invalid.

Any feedback would be appreciated.

The consequence of this is that we'd have to keep all the graphs up-to-date whenever we change the schema. It feels nice to have all the graphs in the most current format, but is it necessary?

I added tools/checkGraph.py to do the JSON validation of each file. But we may be able to replace this with a command line JSON validator (pip install json-spec ?). In that case we'd just have to recognise and skip the physical graphs, or validate them separately using the appropriate schema.

coveralls commented 2 years ago

Coverage Status

Coverage: 81.843% (-0.03%) from 81.87% when pulling e6b77e2037330821f5e6eee9b89cf462e0b59903 on validate-graphs into 22c8c6fd049c84ecf927eb96ce1ad982e56a708a on master.

james-strauss-uwa commented 2 years ago

I know most of these graphs are used for various tests, perhaps we could work out how to programmatically generate these cases instead, so that they are correct by default, rather than maintaining them manually which could definitely be tedious.

I like this idea. We could use the JSON schema to (partially?) generate the JSON. Maybe using something like this: https://github.com/ghandic/jsf

pritchardn commented 2 years ago

I'll write up a ticket and assign to myself, hopefully I can get to this in the next week or so.

james-strauss-uwa commented 2 years ago

Sorry, I've already added a ticket: https://icrar.atlassian.net/browse/LIU-325

I assigned it to myself, but feel free to take it if you have time. I won't tackle it immediately

james-strauss-uwa commented 1 year ago

I just noticed this out-of-date PR and synced it with master.

@awicenec Could you take a look and see if this is still useful, or if it is even required?

awicenec commented 1 year ago

Will merge and close this once PR220 has been accepted

awicenec commented 1 year ago

This has now been merged through PR liu-338 into master, will thus close this one.