[Crates] Only use non-yanked crate versions (ready for merge)

[johannesvollmer]

No description provided.

[github-actions]

	Messages
📖	✨ Thanks for your contribution to Shields, @johannesvollmer!

Generated by 🚫 dangerJS against 65d2826

[johannesvollmer]

is there a way to add tests to this, without setting up a special crate? can I test with a mocked response json? (the current json for the exrs crate)

[johannesvollmer]

Use max version instead of newest version? What's the difference?

other code in this repo uses
json.crate.max_stable_version ? json.crate.max_stable_version : json.crate.max_version

[johannesvollmer]

Same issue exists elsewhere in repo. Apply a fix in the shared base class?

shields/services/crates/crates-license.service.js

Line 40 in e148c62

const license = version ? version.license : versions[0].license

[chris48s]

Thanks for picking this up.

Same issue exists elsewhere in repo. Apply a fix in the shared base class?

Yes. I think we should declare a function on BaseCratesService to get the latest non-yanked version. Then we should use that same definition of "latest" when we want to fetch any data point for the latest version, so:

• MSRV
• License
• Number of downloads

should all get the "latest" version in the same way (and all exclude yanked versions).

[johannesvollmer]

• put the logic for finding the correct version info into one function and reuse it everywhere
• use crate.max_version, as it is the only field seems to be required in the response
• deduplicate response scheme

[johannesvollmer]

0204130921 Error: I've made a huge mistake

What does that mean? What exactly did go wrong? How can I make this unit test run? Please help.

[chris48s]

Thanks for making a start on this. Rather than coach you through this one, I've decided to just take what you've done as a baseline and work from there, retaining the core approach. I've pushed my proposed solution to this branch.

There are some changes I've made which are just cleanup, getting the build to pass, adding tests, etc. To summarise the key changes:

• I think it makes sense to base everything (license, downloads, MSRV) off the latest stable version if there is one
• I've moved the helper function(s) onto BaseCratesService
• I've just written the tests as unit tests on base class

Does this implementation seem reasonable to you?

Just to clarify a few points where you seemed to be getting stuck:

1. There are 2 types of this test in codebase: The ones in files called .tester.js are integration tests: We bootstrap the app and test it from the outside by making HTTP calls against it. Most of these tests touch live services, but we can also use mocks. Then the tests in files called .spec.js are unit tests that test the code. They run using 2 different test frameworks. One of the reasons you were hitting problems was because you were trying to write a unit-style test (that tests an individual function) in a .tester.js.

Error: I've made a huge mistake

What does that mean?

There are some tests in the core test suite that test some code which outputs something to the console, and we don't suppress console output under test. I guess you're seeing this when running.
https://github.com/badges/shields/blob/master/core/base-service/base.spec.js#L225-L244

3. The purpose of validating the response we receive against a schema is so that we can write the rest of the code on the assumption we have an object that conforms to that schema. That allows us to be a bit less defensive in the code we write post-validation. If we don't have an object that conforms to the expected format, we'll throw when we validate it.

Hope some of that helps?

[johannesvollmer]

thanks for making it work <3 I have nothing else to add. your points sound reasonable and are a welcome addition.

[johannesvollmer]

Well, one bike shedding addition: As it seems difficult to decide whether to use max_version or max_stable_version, maybe a different PR might want to read this from a url parameter, allowing the user to decide. I'd not do it right now in this PR though, to keep the scope narrow. For now, max_stable indeed seems like the best default.

[chris48s]

OK, cool. I'm going to merge what we've got for now.

I think this is pretty much in line with what we do for other package registries.