Add simple tests for yamlprobe #1479

yuumasato · 2020-02-13T17:36:51Z

Description

Add a few simple tests using yamlfilecontent_test to exercise not-yet-existing yamlfileprobe (time to TDD).

tests/probes/yamlfilecontent/test_probes_yamlfilecontent.sh

yuumasato · 2020-02-13T17:40:34Z

@evgenyz @matejak I'd appreciate any feedback on the tests and the direction it is going.

tests/probes/yamlfilecontent/test_probes_yamlfilecontent.sh

evgenyz · 2020-02-13T17:59:07Z

tests/probes/yamlfilecontent/test_probes_yamlfilecontent.xml

+    <ind-def:yamlfilecontent_object version="1" id="oval:0:obj:1">
+      <ind-def:path>/tmp</ind-def:path>
+      <ind-def:filename>openshift-logging.yaml</ind-def:filename>
+      <ind-def:yamlpath>/kind</ind-def:yamlpath>


Current YAML Path definition is here: https://github.com/evgenyz/libyaml-yamlpath-filter/wiki/YAML-Path-(OVAL-subset). It became even smaller; for now there are only keys, indexes and slices (for lists, not for maps). And only dot notation (to be more compatible with JSONPath).

You can get the idea of what it is capable of right now here: https://github.com/evgenyz/libyaml-yamlpath-filter/blob/afb1fec90a5eb3974d1115bab1d1bfff4956b91a/test-paths.c#L170.

for now there are only keys, indexes and slices (for lists, not for maps).

I'm not sure if array slicing will be that useful. Can we be sure of the order of the items in a list?

So hash key searching is not supported? 😢
I think key searching will be the most useful feature.

There is a case already for slices (which could be used as any filter for sequences), in the example we have we are looking for at least one record of certain type. The path is .spec.pipelines[:].inputSource and result would be ['logs.app', 'logs.infra', 'logs.audit']. We are after logs.audit.

Key searching is another thing. You can address anything by an exact key. But there is no fuzzy key search of any kind right now. Until we prove it to be useful. I try to keep it as dumb as possible to actually have all the logic in OVAL territory.

Can we be sure of the order of the items in a list?

Yes, it'd be the same as in the document.

Lets just stick to what we have right now, and evolve with the content, should the need arise.

I understand, we need to start writing content as soon as possible then, to collect use cases, even when the probe is not ready.

But I want to be sure that workaround is ugly enough or impossible before starting to deal with all the problems related to [XXX=YYY].

At the same time, one of the goals of the probe is to avoid need for workarounds or complex OVAL contents.

@evgeni Apologies, 😮

No worries :)

At the same time, one of the goals of the probe is to avoid need for workarounds or complex OVAL contents.

Balance. That is what we usually looking for :)

I understand, we need to start writing content as soon as possible then, to collect use cases, even when the probe is not ready.

Exactly. So I'm trying to create something to give our OCP colleagues tools to start working and pinpointing weak spots.

Is the python style slicing syntax specific to libyaml-yamlpath-fitler?
yaml-get doesn't like it.

Yes, it is. And it is how things are done in JSONPath. I don't like YAML Paths' version because you can't collect the whole list with it.

YAML Paths' '.spec.pipelines.inputSource' is too ambiguous. It completely skips the list in the middle, inputSource may as well be just a key in pipelines dictionary, which is not acceptable at all. It is so because YAML Path uses so called arrays pass-through, which is not good from a security-related tool point of view, IMHO.

BTW, if you want to play with our (he-he) YAML Path, go ahead: https://github.com/OpenSCAP/yaml-filter. We even have a console tool now :)

tests/probes/yamlfilecontent/test_probes_yamlfilecontent.xml

Key searching is not supported

evgenyz · 2020-02-15T06:55:56Z

I feel like you overdoing this right now. We have unit tests for YAML Path behaviour in the library. We are after the probe integration tests: no results, 1 result, multiple results, errors (no file, invalid path, path pointing to a non-scalar value).

jan-cerny

I think we have a perfect testing OVAL content now, but we should also check that probe collects the data from the YAML file.

tests/probes/yamlfilecontent/test_probes_yamlfilecontent_array.sh

tests/probes/yamlfilecontent/test_probes_yamlfilecontent_key.sh

tests/probes/yamlfilecontent/test_probes_yamlfilecontent_array.sh

tests/probes/yamlfilecontent/test_probes_yamlfilecontent_array.xml

tests/probes/yamlfilecontent/test_probes_yamlfilecontent_key.sh

jan-cerny · 2020-02-17T09:32:27Z

tests/probes/yamlfilecontent/test_probes_yamlfilecontent_key.xml

+      <ind-def:state state_ref="oval:0:ste:2"/>
+    </ind-def:yamlfilecontent_test>
+
+    <ind-def:yamlfilecontent_test version="1" id="oval:0:tst:3" check="all" comment="error">


Why is error expected?

Because the path .metadata on the test yaml file will return an non-scalar: https://github.com/OpenSCAP/openscap/pull/1479/files/68289f537fbb8607212c56e6300d413af24b24be#diff-5ca741cc9861804e1ff83bfe153329cbR3

jan-cerny · 2020-02-17T09:36:12Z

tests/probes/yamlfilecontent/test_probes_yamlfilecontent_key.sh

+
+    rm -f $YAML_FILE
+
+    return $ret_val


Where do you test that the correct data was extracted from the YAML? Please add assertions on collected items.

The tests have states, if they pass doesn't it mean we have collected the correct data?

Probably yes

What about the that expected error result? And what about that non-existing scalar? The test could assume that no objects are collected.

Per yamlpath description in https://github.com/OVAL-Community/OVAL/pull/90/files#diff-ae56180beddebc6013e4387e9ed9fe0eR2118, error is the expected result when the path leads to a non-scalar.

The xpath field in xmlfilecontent_object behaves the same way.

Oh, maybe @matejak is asking about how the test verifies that the result was error?
In verify_results() the result of evaluation is compared with the results the comments.

Thanks, I was not able to understand just by looking at verify_results documentation. That doc needs a refresh.

tests/probes/yamlfilecontent/test_probes_yamlfilecontent_key.xml

By updating the scheman version to a future one, and providing updated "future" schemas, we can overcome issues with schema validations.

tests/probes/yamlfilecontent/test_probes_yamlfilecontent_array.xml

tests/probes/yamlfilecontent/test_probes_yamlfilecontent_key.xml

yuumasato · 2020-02-17T13:52:32Z

I feel like you overdoing this right now.

I think we have a perfect testing OVAL content now,

😃

We have unit tests for YAML Path behaviour in the library. We are after the probe integration tests: no results, 1 result, multiple results, errors (no file, invalid path, path pointing to a non-scalar value).

@evgenyz I don't understand what you are missing.
I believe these work as integration tests, and most of what you mention is already covered.

matejak · 2020-02-17T15:28:43Z

@evgenyz I don't understand what you are missing.
I believe these work as integration tests, and most of what you mention is already covered.

I think that Evgeny is not happy with the library being tested twice - once in respective unit-tests and basically once by analysing OVAL results produced by the probe that uses the library.
I am not against this "double testing", as I find it as an appropriate "black box" testing of the probe.

matejak · 2020-02-17T15:47:15Z

tests/probes/yamlfilecontent/test_probes_yamlfilecontent_key.xml

+    <ind-def:yamlfilecontent_object version="1" id="oval:0:obj:5">
+      <ind-def:path>/tmp</ind-def:path>
+      <ind-def:filename>yaml-filter.yaml</ind-def:filename>
+      <ind-def:yamlpath>.foo[1].some\.el\/here</ind-def:yamlpath>


The test yaml file modification is needed in order for this to pass, right?

Actually back-slash escaping is not supported so far. This won't work no matter what, ATM.

@evgenyz Do we expect to support it?

The test yaml file modification is needed in order for this to pass, right?

I forgot to add yaml-filter.yaml into the commit.
But I'll drop the commit for now.

Yes, we expect. Working on it. But you'd better not include it in this PR. (Edit: Actually it could be a bit different)

evgenyz · 2020-02-17T20:26:57Z

@evgenyz I don't understand what you are missing.
I believe these work as integration tests, and most of what you mention is already covered.

I think that Evgeny is not happy with the library being tested twice - once in respective unit-tests and basically once by analysing OVAL results produced by the probe that uses the library.

Yeah, I'm grumbling about excessive tests, not because something is missing. While excessive test might be seemed as a neutral or even a positive feature, one should always keep in mind that there is a toll of test maintenance (and execution time, BTW) in a developing library (such as libyamlpath) and usually there are no gains from this duplicity.

The comment had the id, and id was empty

yuumasato · 2020-02-18T12:32:31Z

Yeah, I'm grumbling about excessive tests, not because something is missing. While excessive test might be seemed as a neutral or even a positive feature, one should always keep in mind that there is a toll of test maintenance (and execution time, BTW) in a developing library (such as libyamlpath) and usually there are no gains from this duplicity.

I'm happy to remove anything that is excessive and duplicate.
But just like Matěj, I see these as black box tests for the probe. They will define expected behavior of the probe and how content would look like. They are not focused in finding bugs in yaml-filter.
CC: @matusmarhefka, maybe you have opinions ^

yuumasato · 2020-02-18T12:34:21Z

Per #1479 (comment) I've dropped escaped key test: b61d5f9

yuumasato · 2020-02-18T12:48:53Z

I was going to add tests for anchor, but @evgenyz said it is not implemented yet.

So this should be good to merge, if reviewers are ok.

jan-cerny · 2020-02-18T14:04:07Z

I'm fine.

evgenyz · 2020-02-18T14:10:09Z

I'm fine.

But it's your request for changes still active :)

evgenyz · 2020-02-18T14:10:54Z

I'm fine with this PR, don't take my whining about excessiveness as a blocker

Add simple tests for yamlprobe

55ab772

yuumasato commented Feb 13, 2020

View reviewed changes

tests/probes/yamlfilecontent/test_probes_yamlfilecontent.sh Outdated Show resolved Hide resolved

evgenyz reviewed Feb 13, 2020

View reviewed changes

jan-cerny reviewed Feb 14, 2020

View reviewed changes

tests/probes/yamlfilecontent/test_probes_yamlfilecontent.xml Outdated Show resolved Hide resolved

yuumasato added 3 commits February 14, 2020 14:42

Drop instance element and update yaml path syntax

4bff23c

Rewrite test content to use array slices

9af6e7b

Key searching is not supported

Split tests by features of yamlpath

68289f5

jan-cerny requested changes Feb 17, 2020

View reviewed changes

yuumasato commented Feb 17, 2020

View reviewed changes

tests/probes/yamlfilecontent/test_probes_yamlfilecontent_key.xml Outdated Show resolved Hide resolved

yuumasato added 5 commits February 17, 2020 13:14

Fix typo in yamlfilecontent_key.xml

bdc3cfe

Fix indentation and syntax of test script

94663a3

Add permission to execute tests script

73ce2d0

Update schema of yamlfilecontent OVAL to 5.11.3

e503b10

By updating the scheman version to a future one, and providing updated "future" schemas, we can overcome issues with schema validations.

Add test for non existing scalar

bdcf841

jan-cerny requested changes Feb 17, 2020

View reviewed changes

matejak reviewed Feb 17, 2020

View reviewed changes

yuumasato added 2 commits February 18, 2020 13:21

Fix id and comment of variable in array test

3a16c41

The comment had the id, and id was empty

Fix typo in key value_of

d54d078

yuumasato force-pushed the add-yamlfilecontent-test branch from 8231672 to d54d078 Compare February 18, 2020 12:24

yuumasato marked this pull request as ready for review February 18, 2020 12:48

jan-cerny approved these changes Feb 18, 2020

View reviewed changes

evgenyz merged commit 97a19ac into OpenSCAP:maint-1.3 Feb 18, 2020

yuumasato deleted the add-yamlfilecontent-test branch February 18, 2020 14:19

yuumasato added this to the 1.3.3 milestone Feb 18, 2020

Add simple tests for yamlprobe #1479

Add simple tests for yamlprobe #1479

Conversation

yuumasato commented Feb 13, 2020

Description

yuumasato commented Feb 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evgenyz Feb 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuumasato Feb 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evgenyz commented Feb 15, 2020

jan-cerny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuumasato commented Feb 17, 2020

matejak commented Feb 17, 2020 • edited by evgenyz Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evgenyz Feb 18, 2020 • edited Loading

Choose a reason for hiding this comment

evgenyz commented Feb 17, 2020 • edited Loading

yuumasato commented Feb 18, 2020

yuumasato commented Feb 18, 2020

yuumasato commented Feb 18, 2020

jan-cerny commented Feb 18, 2020

evgenyz commented Feb 18, 2020

evgenyz commented Feb 18, 2020

evgenyz Feb 14, 2020 •

edited

Loading

yuumasato Feb 14, 2020 •

edited

Loading

matejak commented Feb 17, 2020 •

edited by evgenyz

Loading

evgenyz Feb 18, 2020 •

edited

Loading

evgenyz commented Feb 17, 2020 •

edited

Loading