test: add tests for ruby

[EslamHiko]

This test makes sure that ruby importer works!

[sbs2001]

The test is actually doing the whole import process minus the making the db entries. Can you maybe, make the tests do less work, like check for some select few advisory entries rather than the whole advisory repo.

The way I see these tests will be of value, is when the schema of the advisory is changed. The tests should just do that.

[EslamHiko]

@sbs2001 the break here let the test checks only the first package : https://github.com/nexB/vulnerablecode/pull/168/files#diff-9ccae1b60aeb7dece31565f5f7284b0eR39
Yes, that's the point of the test because this issue happened before for rust : #154

[sbs2001]

That is correctly done , the issue is RUBYSEC_DB_URL. Basically running the test, despite having the break will result into downloading the whole repo(which won't be used), since the repo is tiny, I don't mind merging this as it is, but there is a room for improvement .

What I had in mind was to load specific advisories from the repo like https://raw.githubusercontent.com/rubysec/ruby-advisory-db/master/gems/actionpack/CVE-2016-0751.yml for testing purpose

[EslamHiko]

@sbs2001 I understand now what you mean, but I think leaving it as it is will be better because it's testing rubygem_advisories function too, the folder structure may change after many years 😅, also I don't mind writing an importer for a single yml file.

@haikoschol, what do you think?

[haikoschol]

I think tests that run in CI and prevent merging when they fail should not depend on anything other than the code in the repository. So if we have tests like this one, they must be disabled in CI. If they are disabled in CI, the question is when and how are they run?

But the first question is: Why do we need tests like this at all? What exactly happened with Rust? Did they define a schema and then just change it without any kind of versioning or deprecation mechanism? Or does our code consume some data that does not actually come with any contract regarding its structure?

[sbs2001]

@haikoschol

What exactly happened with Rust? Did they define a schema and then just change it without any kind of versioning or deprecation mechanism?

Yes, they changed the schema. I wouldn't say they did that out of blue, they actually announced it rustsec/advisory-db#228 , seems we need to keep tabs on all advisories, which is what these tests aim to do :)

I think tests that run in CI and prevent merging when they fail should not depend on anything other than the code in the repository

We could run such tests only , now that we have offline tests option, but then again it won't be assured whether VulnerableCode is working(we are dependent on external factors).

@EslamHiko

because it's testing rubygem_advisories function too, the folder structure may change after many years

rubygem_advisories is 'folder-structure' agnostic.

[haikoschol]

What exactly happened with Rust? Did they define a schema and then just change it without any kind of versioning or deprecation mechanism?

Yes, they changed the schema. I wouldn't say they did that out of blue, they actually announced it RustSec/advisory-db#228 , seems we need to keep tabs on all advisories, which is what these tests aim to do :)

I looked into this as well today and found the tickets and PRs about it. It seems this was not a random change that can be expected to occur often. They are aware that this breaks third parties that consume the data and try to keep schema changes to a minimum and when they happen, announce them. They also say to get in touch if necessary, for some kind of coordination, to avoid breakage the next time. I think that's reasonable and having an importer break occasionally is not a big deal.

I understand that these tests are meant to detect that case early. But running them in CI does not achieve that. It just detects it whenever CI is run the next time. As long as there are no changes to VulnerableCode, CI is not run.
Additionally, whenever some data source which is required for these tests is temporarily unavailable, we can't merge code, even if it is completely unrelated to that data source.
When VulnerableCode is deployed somewhere and importers are being run periodically there must be some kind of alerting when they fail anyway.

But this is just my opinion and I shared it because I was asked for it. I don't actually feel strongly about it either way. @EslamHiko @sbs2001 if you want it to be merged, I'll merge it.

[EslamHiko]

Additionally, whenever some data source which is required for these tests is temporarily unavailable, we can't merge code, even if it is completely unrelated to that data source.

I think The test will detect the unavailability, when it's detected we can investigate it, is the reason for the unavailability in our code? did they change something?
If the reason is fixable we can submit the fix patch if it's not we can temporarily disable the test & the importer as the importer then won't import data, and merge the other importers.

[haikoschol]

That's the point, the reason is not in our code. No network service has 100% uptime. When they are not available it makes no sense that this impedes us from merging code. Our test will not detect anything useful in that case. Disabling an importer because its' data source is temporarily unavailable also doesn't make sense because you can't disable importers of deployed instances of the app that are controlled by whoever deployed them. And you don't want to, because eventually the import will be run again after that network service has become available again.

[EslamHiko]

@haikoschol I got it, I think I'll go with @sbs2001 suggestion and test a .yml file but locally.

[haikoschol]

Why duplicate this variable instead of importing it?

[sbs2001]

Closed due to #185