Fixes #37613 - compare parsed YAML results, not Strings

[evgeni]

depending on the libyaml version, it encodes nil differently and thus
the tests fail on newer versions as the string comparison fails.
if we parse the generated yaml back into a Hash, things compare just
fine again, without breaking the tests on older libyaml versions.

[evgeni]

this could equally be

Suggested change

	assert_equal expected_yaml, YAML.safe_load(@scope.report_render(format: :yaml))
	assert_equal expected_yaml.to_yaml, @scope.report_render(format: :yaml)

but I think that's uglier?

[ekohl]

I'll let @ares give the final ack.

[ekohl]

Since @ares is out this week, I'm merging this now.

[ares]

@evgeni we should document this change of handling nil in the release notes. By using to_yaml in the test, we'll no longer be able to detect such changes.

[evgeni]

What do you mean by "change of handling" (there was no code change in this PR)? The only thing changing (depending on the libyaml version) is the generated on-disk representation, both libyaml versions parse "the other" format just fine to nil.

[ares]

I may be misunderstanding the change... the newer libyaml stopped representing nil as nil but keeps the value empty in the dumped YAML (or the other way around). With certain version of Foreman we'll start shipping newer version of libyaml. From that moment, users will no longer see nil in their YAML report, but empty value or null or whatever the libyaml now prefers.

This patch avoids detecting such change by making the part of the test dynamic. Instead of comparing the snapshot of generated result, it takes a structure with an empty value and performs .to_yaml on it to generate a "snapshot" live. That means it will use the same logic, the same version of libyaml. I guess that's acceptable for unit test because we shouldn't be testing libyaml here. But we no longer have an e2e test, that would verify the end result and warn us about any change like this.

Based on quick search, other programming languages may not parse nil correctly. It's more common to use null or empty value. The YAML implementations in other languages are not always 100% correct. If someone extracts the data in YAML from Foreman, I guess it is for further machine processing and chances are, it's not Ruby that processes it.

I don't know how big deal it really is, I never heard about users of this format. But it is a change in how we represent empty value in all reports, hence I suggested to mention it in the release notes.

I hope I provided more context.

[evgeni]

I may be misunderstanding the change...

Yepp ;-)
Let's see if I can untangle that.

the newer libyaml stopped representing nil as nil but keeps the value empty in the dumped YAML (or the other way around).

In YAML there is no nil -- that's a Ruby thing. In YAML you can do null or ~.
This is e.g. what Python does:

>>> yaml.dump({'something':None})
'something: null\n'

However, it is also completely legal to leave the value totally empty, which is what Ruby does:

irb(main):002> YAML.dump({'something':nil})
=> "---\n:something:\n"

Now, what changed is how empty the string representation exactly is (the above is on libyaml 0.2.x) from 0.1.x:

irb(main):002:0> YAML.dump({'something':nil})
=> "---\n:something: \n"

You see, the value is in both cases empty, but in the past the string dump contained a space after the colon (as a separator, like you do for any non-empty value), and now it does not. For the parser this is totally irrelevant, but the strings do differ.

With certain version of Foreman we'll start shipping newer version of libyaml.

We don't ship libyaml, that's the OS (EL8, EL9, Debian, etc).

From that moment, users will no longer see nil in their YAML report, but empty value or null or whatever the libyaml now prefers.

See above, they never did see a (string) nil.

This patch avoids detecting such change by making the part of the test dynamic. Instead of comparing the snapshot of generated result, it takes a structure with an empty value and performs .to_yaml on it to generate a "snapshot" live. That means it will use the same logic, the same version of libyaml. I guess that's acceptable for unit test because we shouldn't be testing libyaml here. But we no longer have an e2e test, that would verify the end result and warn us about any change like this.

But as long it's a valid YAML document, should we care?

Based on quick search, other programming languages may not parse nil correctly. It's more common to use null or empty value. The YAML implementations in other languages are not always 100% correct. If someone extracts the data in YAML from Foreman, I guess it is for further machine processing and chances are, it's not Ruby that processes it.

As noted above, nil is not something that YAML understands:

irb(main):003:0> YAML.load('something: nil')
=> {"something"=>"nil"}

It gets parsed as a string!

For all other notations, other languages should be fine, as long they implement the spec correctly:

>>> yaml.safe_load("---\n:something: \n")
{':something': None}
>>> yaml.safe_load("---\n:something:\n")
{':something': None}
>>> yaml.safe_load("---\n:something:")
{':something': None}
>>> yaml.safe_load("---\n:something: null")
{':something': None}
>>> yaml.safe_load("---\n:something: nil")
{':something': 'nil'}

And we should really not care for people who do not have a spec-compliant parser [1][2][3]

[1] https://yaml.org/spec/1.2.2/#72-empty-nodes
[2] https://yaml.org/spec/1.2.2/#10211-null
[3] https://yaml.org/spec/1.2.2/#1032-tag-resolution

I don't know how big deal it really is, I never heard about users of this format. But it is a change in how we represent empty value in all reports, hence I suggested to mention it in the release notes.

Again, we don't change this (this PR only adjusts tests, no actual code), as we do not ship libyaml (or Psych, or Ruby, etc). It's solely dependent on the platform the user deploys Foreman on.

I hope I provided more context.

Same :)

[ekohl]

Again, we don't change this (this PR only adjusts tests, no actual code), as we do not ship libyaml (or Psych, or Ruby, etc). It's solely dependent on the platform the user deploys Foreman on.

And the reason we hit this was that we updated our CI nodes from EL8 to EL9, changing libyaml. It was never a guarantee we gave.