search

Amartus / yang2swagger

Yang to swagger generator
Eclipse Public License 1.0
32 stars 21 forks source link

Create bug reproducer #56

Closed ihrasko closed 2 years ago

ihrasko commented 2 years ago

Adapt tests to prove that grouping definitions dont follow hierarchy of the module but instead they depends on grouping naming.

When we change groupingroot.G1 to groupingroot.G4 we are no more able to get its definition because its is replaced by previously "rewritten" G1 grouping which was not defined without this change:

Try to run test without this patch - you can see that we have definition for G2 but not for G1, instead we have groupingroot.G1!

Signed-off-by: Ivan Hrasko ivan.hrasko@pantheon.tech

ihrasko commented 2 years ago

@bartoszm FYI: It works when we change G1 to G0 as in https://github.com/bartoszm/yang2swagger/pull/53/commits/fbff0d1c53644d77b9ddfcda648d961fb6912770. Then definition of groupingroot.G1 is available.

P.S. Its required with yangtools 6.0.7 to use unique grouping names.

bartoszm commented 2 years ago

Actually with the change you propose to yang G4 should never be present in the resulting swagger - because it is not used. The whole point of the original test was to check that the internal that the "closes" grouping definition is used --> not the root level one. What you are saying is that 6.0.7 is not supporting that any longer - which is fine I guess as long as we document that in the release notes.

bartoszm commented 2 years ago

@ihrasko please feel free to close this reproducer if you agree with my explanation. We can handle test modification to 6.0.7 requirements in your second PR

bartoszm commented 2 years ago

Hi @ihrasko, any comments on above?

ihrasko commented 2 years ago

"What you are saying is that 6.0.7 is not supporting that any longer - which is fine I guess as long as we document that in the release notes."

Just to clarify this - this reproducer means that the behaviour is the same in 'master' branch, too. It does not mean that yangtools 6.0.7 has changed anything.

If this behaviour is not a bug - then yes, we can close this PR.

bartoszm commented 2 years ago

There was a bug not in behavior but in test assertions themselves. I will close this tracker. I have one issue I need to look at with RCP I want to look at. Later I will introduce your changes in the second PR, which will be a breaking change in terms of behavior but that's OK. Once again thank you a lot for your contribution and apologise for the delays.