Stop showing documentation link when documentation is removed from Cargo.toml #948

sunjay · 2017-08-11T07:20:42Z

Fixes #945

This is exactly the fix suggested in #945 (comment) but applied to the NewCrate struct instead of the Crate struct since NewCrate is actually what gets stored in the DB.

Problems with this fix:

The webpage for the crates that are affected by this will only get updated when they next publish
The version specific webpages for the crate do not show a documentation link even if that crate once had a documentation key
- This is because we do not track this metadata per version and should probably be a separate issue

sunjay · 2017-08-12T04:06:41Z

This is the first time I'm working with this codebase (and with diesel, etc.) so please forgive me (and correct me) if I make any errors. 😄

I'm looked into this in more detail now and I agree that the fix proposed in @sgrif's comment should fix this issue. After some debugging and understanding the code a bit more, I moved the suggested line to the actual spot it needed to go to and it worked.

So now if you publish a crate and remove the documentation link from Cargo.toml that link will disappear off of the crate page. The problem is that because we don't track that information per version, if you click on any of the other versions to inspect them, you aren't able to see any documentation link.

I'm not sure if you want to fix this as part of this PR, but it should probably get an issue of its own. The migration to fix that is going to be substantial... 😨

Note This fix only updates the metadata when you update the crate. Any existing crates that are wrong will not be fixed. So the person from #945 will need to publish a new version before the crates.io page gets updated.

Here's an example:

testcrate has no documentation key in its Cargo.toml in version 0.1.0 - no documentation link shows up on crates.io
testcrate adds a documentation key in version 0.1.1 - documentation link shows up on crates.io
testcrate removes the documentation key in version 0.1.2 - documentation link stops showing up on crates.io but also stops showing when we view the page for version 0.1.1

I'll summarize all of this in the PR description above.

sunjay · 2017-08-12T04:36:34Z

Also we might want to add a test for this. I don't know how to do that in this codebase, so if you think that's necessary please give me an idea of what to do and I'll go and do it. 😄

carols10cents · 2017-08-12T15:26:03Z

This is because we do not track this metadata per version and should probably be a separate issue

See #894 for this issue.

Also we might want to add a test for this. I don't know how to do that in this codebase, so if you think that's necessary please give me an idea of what to do and I'll go and do it. 😄

Yup, I think we need a test to make sure we don't break this again! What have you tried so far with the tests?

I would expect a new test to be in src/tests/krate.rs, and consist of:

Using CrateBuilder, create a crate in the database that has a documentation URL (see other tests in that file for examples of using CrateBuilder
Publish a new version of the crate through the API, without sending a documentation URL
Request the crate information and assert that the documentation URL isn't present

This test is a pretty good example of adding a crate to the dabatase and then publishing another version of that crate through the API, it uses some helper functions to do that: https://github.com/rust-lang/crates.io/blob/master/src/tests/krate.rs#L657-L673

The show test demonstrates how to request and assert on the crate information: https://github.com/rust-lang/crates.io/blob/master/src/tests/krate.rs#L390-L416

Please let me know if you have any other questions!

sunjay · 2017-08-12T22:05:39Z

@carols10cents I'm having trouble working with the RECORD environment variable. After many errors about headers not matching up with the cached http-data file, I figured out that I need to set RECORD=1 and delete the current cached file to get the tests to try to cache again. The problem is that whenever I try to record, the test just hangs and nothing ever gets recorded. Have you run into this before? Do you know what usually causes this?

The test hangs on a line like this:

        let mut response = ok_resp!(middle.call(&mut req));

More specifically, this line in the recorder tries to read the socket to its end and doesn't ever seem to get there. This happens in the second request of my test. For some reason the first request works, but then the second fails. Are tests limited to a single request?

I'm not doing anything drastically different from any of the tests you showed me as examples. I'm not sure what's going wrong.

carols10cents · 2017-08-13T00:19:53Z

Oh sorry, I forgot the publish tests make a request to S3 that needs to be recorded! Rather than setting up an S3 bucket, I usually copy another recorded http-data file and change the crate name as necessary.

sunjay · 2017-08-13T03:58:54Z

src/tests/krate.rs

+    }
+
+    // Ensure latest version also has the same documentation
+    {


Notice that I've done the same assertion twice. Once to make sure the response of the PUT request is correct, and another to make sure that the latest information returned for the crate is correct. Both are necessary to make sure this code works correctly no matter what.

sunjay · 2017-08-13T04:03:28Z

@carols10cents Thanks! I was able to figure it out based on what you said.

I added two tests for this fix. One of them is a basic test and ensures the functionality required to fix #945. The other is a more advanced version of the same test which should be verified in #894. The behaviour being tested there is that the information for the latest crate version (the max version) doesn't get changed by publishing a lower version. This doesn't work yet, so the test is ignored. When that other issue is fixed, that test should be enabled as part of it because once we're properly tracking this information per version, we should be able to keep everything up to date properly.

sunjay · 2017-08-13T04:47:48Z

@carols10cents I don't know what the solution to the build error is. It's some kind of linking problem or something. Could you look into it? Maybe the build just needs to be rerun?

carols10cents · 2017-08-13T14:25:22Z

@sunjay sorry about that, can you try rebasing on master? Travis is running out of memory, we were seeing this over on #869 too and the commit I just cherry picked onto master, to change sudo: false to sudo: required in the travis config, gives us more memory in the travis env for some reason. I've reported this over at rust-lang/rust#43789 too.

sunjay · 2017-08-13T18:53:56Z

@carols10cents I've rebased. The build passes now! 😄

carols10cents · 2017-08-22T01:19:51Z

src/tests/krate.rs

+        assert_eq!(json.krate.documentation, None);
+    }
+
+    // 4. Add documentation again to make sure this change isn't permanent


I think this test is a bit overzealous :) I can't think of a scenario where we would accidentally change the code to allow adding and removing documentation once but not allow you to add the documentation link again after removing... so I think from line 1512 through 1549 could be removed without loss in test coverage. Could you make that change please?

carols10cents · 2017-08-22T01:21:22Z

src/tests/krate.rs

+    }
+
+    // Verify that crates start without any documentation so the next assertion can *prove*
+    // that it was the one that added the documentation


Similarly, this block (L1568-1575) tests the same thing that the previous test does in L1464-1471. I can't think of a scenario where one of them would pass and the other would fail. Could you remove this block please?

carols10cents · 2017-08-22T01:23:48Z

src/tests/krate.rs

+        assert_eq!(json.krate.documentation, Some("http://foo.rs".to_owned()));
+    }
+
+    // 3. Remove the documentation


And again, imo lines 1637-1691 are not providing any additional coverage. Please remove, thank you!

carols10cents · 2017-08-22T01:24:43Z

I tested this out and it works great-- I just think the tests are a bit too much :) Please let me know if you'd rather that I just take care of this, or if you have any questions! Thanks!

sunjay · 2017-08-22T02:06:28Z

@carols10cents Sorry if the intention in my test isn't clear! Thanks for your feedback!

The test is designed to give us confidence that adding or removing the documentation will work across versions, even if it is added or removed multiple times over a project. This has more to do with testing a realistic scenario than just making sure the lines are covered. You're right in that the second part of both of those tests won't change the test coverage. It's reasonable to say that over the long life of a project the documentation can be added or removed a few times. Now, it doesn't make sense to do that over and over again a crazy number of times in a test, so I've just done it twice. The test adds documentation, removes it, and then adds it and removes it again. The lines you highlighted are the second part of that scenario.

I think it's a good idea to make sure this works multiple times in case something breaks if we ever track documentation by version or make any other change that updates the documentation conditionally based on version. In particular, the test that does not contain _basic in its name tests some pretty important behaviour which I'll talk about more in a bit. (#894 suggests we won't do that for now, but I imagine we may need something similar to address the issues this causes.)

If you still think I should remove that second part of each of the tests, I will, but I think it's a good idea to keep it so we know this scenario works well. I'll leave that to your judgement.

I can't think of a scenario where one of them would pass and the other would fail.

I agree! These tests are actually one test that I had to simplify and split in order to get everything to pass for now. The test with _basic in its name could actually be removed entirely at some point. The other test is ignored for now with #[ignore]. The difference between the tests is that the non-basic version tests if publishing a lower version can accidentally overwrite the documentation of the max version. This is an important property which doesn't actually work right now, but should eventually. (maybe after #894?)

It might also be that I don't understand your feedback, so please let me know if that is what comes across after my explanation. 😄

carols10cents · 2017-08-23T15:23:23Z

It's reasonable to say that over the long life of a project the documentation can be added or removed a few times. Now, it doesn't make sense to do that over and over again a crazy number of times in a test, so I've just done it twice.

To me, it doesn't make sense to even do it twice :) If we did every action in the test suite twice, we'd be doubling the time that every run of the test suite takes for negligible benefit!

I think it's a good idea to make sure this works multiple times in case something breaks if we ever track documentation by version or make any other change that updates the documentation conditionally based on version.

I would expect that the tests would be modified at the point that we change documentation stored per-version, but not before. Who knows, we may never get to making that change, and now we've added time to every run of the test suite from now until forever for no reason. Please remove the duplicated parts.

The other test is ignored for now with #[ignore]. The difference between the tests is that the non-basic version tests if publishing a lower version can accidentally overwrite the documentation of the max version. This is an important property which doesn't actually work right now, but should eventually. (maybe after #894?)

I missed that it was ignored. If the intention is to remove the #[ignore] someday, could you please add a comment detailing the conditions under which the ignore can and should be removed? And remove the duplication between this test and the basic test, and the duplication within the second test.

If you make these changes, I'd be happy to merge this PR.

sunjay · 2017-08-23T16:45:53Z

@carols10cents Sounds good! I will update all of that hopefully in the next few days when I get the chance. 😄

Thanks for all your feedback on this Carol!

sunjay · 2017-12-17T20:24:45Z

Hi @carols10cents! Sorry for the delay! This is done now. There is just a single, minimal test now. My branch was pretty out of date so I copied the changes over to a new one. That's why the history and review comments are a bit messed up.

Should be good to go now. Let me know if you need any more changes. 😄

sunjay · 2018-01-08T23:05:29Z

@carols10cents Hi Carol! Could you take a look at this PR? It should be much smaller and easier to review now. I made the changes you requested a while back, so unless you see more adjustments I should make, this can probably be merged. 😄

Otherwise, it isn't really with documentation :) We can use new_req to create a version without a documentation link.

carols10cents · 2018-03-14T23:12:19Z

Sorry for the delay on this! I made one little tweak but otherwise I think this is ready to go!

bors: r+

948: Stop showing documentation link when documentation is removed from Cargo.toml r=carols10cents Fixes #945 This is exactly the fix suggested in #945 (comment) but applied to the `NewCrate` struct instead of the `Crate` struct since `NewCrate` is actually what gets stored in the DB. Problems with this fix: * The webpage for the crates that are affected by this will only get updated when they next publish * The version specific webpages for the crate do not show a documentation link even if that crate once had a documentation key * This is because we do not track this metadata per version and should probably be a separate issue

sunjay · 2018-03-14T23:14:58Z

Thank you for taking the time! Glad it will be merged! 🎉

bors-voyager · 2018-03-14T23:49:07Z

Build failed

continuous-integration/travis-ci/push

sgrif · 2018-03-15T00:21:28Z

Failure is just from needing to run rustfmt

…

On Wed, Mar 14, 2018, 5:49 PM bors-voyager[bot] ***@***.***> wrote: Build failed - continuous-integration/travis-ci/push <https://travis-ci.org/rust-lang/crates.io/builds/353589954?utm_source=github_status&utm_medium=notification> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#948 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABdWK8qXhtL0EkcSzAnABDl5unTOmeVCks5teax3gaJpZM4O0Uj_> .

carols10cents · 2018-03-15T01:32:44Z

bors: r+

948: Stop showing documentation link when documentation is removed from Cargo.toml r=carols10cents Fixes #945 This is exactly the fix suggested in #945 (comment) but applied to the `NewCrate` struct instead of the `Crate` struct since `NewCrate` is actually what gets stored in the DB. Problems with this fix: * The webpage for the crates that are affected by this will only get updated when they next publish * The version specific webpages for the crate do not show a documentation link even if that crate once had a documentation key * This is because we do not track this metadata per version and should probably be a separate issue

bors-voyager · 2018-03-15T02:02:35Z

Build succeeded

continuous-integration/travis-ci/push

sunjay mentioned this pull request Aug 11, 2017

crates.io shows old documentation link when documentation field removed from Cargo.toml #945

Closed

sunjay changed the title ~~[WIP] Stop showing documentation link when documentation is removed from Cargo.toml~~ Stop showing documentation link when documentation is removed from Cargo.toml Aug 12, 2017

carols10cents added the S-waiting-on-review label Aug 12, 2017

sunjay commented Aug 13, 2017

View reviewed changes

carols10cents requested changes Aug 22, 2017

View reviewed changes

carols10cents added S-waiting-on-author and removed S-waiting-on-review labels Aug 22, 2017

sunjay closed this Dec 17, 2017

Fixes #945

f10bcb5

sunjay reopened this Dec 17, 2017

Fixed clippy errors

b73a797

carols10cents added S-waiting-on-review and removed S-waiting-on-author labels Jan 25, 2018

carols10cents added 2 commits March 14, 2018 18:57

Merge remote-tracking branch 'upstream/master' into sunjay-master

19bd9fd

Have the test fn req_with_documentation always take documentation

adb0715

Otherwise, it isn't really with documentation :) We can use new_req to create a version without a documentation link.

carols10cents approved these changes Mar 14, 2018

View reviewed changes

rustfmt

59da18d

bors-voyager bot merged commit 59da18d into rust-lang:master Mar 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop showing documentation link when documentation is removed from Cargo.toml #948

Stop showing documentation link when documentation is removed from Cargo.toml #948

sunjay commented Aug 11, 2017 •

edited

Loading

sunjay commented Aug 12, 2017

sunjay commented Aug 12, 2017

carols10cents commented Aug 12, 2017

sunjay commented Aug 12, 2017

carols10cents commented Aug 13, 2017

sunjay Aug 13, 2017

sunjay commented Aug 13, 2017

sunjay commented Aug 13, 2017

carols10cents commented Aug 13, 2017

sunjay commented Aug 13, 2017

carols10cents Aug 22, 2017

carols10cents Aug 22, 2017

carols10cents Aug 22, 2017

carols10cents commented Aug 22, 2017

sunjay commented Aug 22, 2017

carols10cents commented Aug 23, 2017

sunjay commented Aug 23, 2017

sunjay commented Dec 17, 2017

sunjay commented Jan 8, 2018

carols10cents commented Mar 14, 2018

sunjay commented Mar 14, 2018

bors-voyager bot commented Mar 14, 2018

sgrif commented Mar 15, 2018 via email

carols10cents commented Mar 15, 2018

bors-voyager bot commented Mar 15, 2018

Stop showing documentation link when documentation is removed from Cargo.toml #948

Stop showing documentation link when documentation is removed from Cargo.toml #948

Conversation

sunjay commented Aug 11, 2017 • edited Loading

sunjay commented Aug 12, 2017

sunjay commented Aug 12, 2017

carols10cents commented Aug 12, 2017

sunjay commented Aug 12, 2017

carols10cents commented Aug 13, 2017

sunjay Aug 13, 2017

Choose a reason for hiding this comment

sunjay commented Aug 13, 2017

sunjay commented Aug 13, 2017

carols10cents commented Aug 13, 2017

sunjay commented Aug 13, 2017

carols10cents Aug 22, 2017

Choose a reason for hiding this comment

carols10cents Aug 22, 2017

Choose a reason for hiding this comment

carols10cents Aug 22, 2017

Choose a reason for hiding this comment

carols10cents commented Aug 22, 2017

sunjay commented Aug 22, 2017

carols10cents commented Aug 23, 2017

sunjay commented Aug 23, 2017

sunjay commented Dec 17, 2017

sunjay commented Jan 8, 2018

carols10cents commented Mar 14, 2018

sunjay commented Mar 14, 2018

bors-voyager bot commented Mar 14, 2018

Build failed

sgrif commented Mar 15, 2018 via email

carols10cents commented Mar 15, 2018

bors-voyager bot commented Mar 15, 2018

Build succeeded

sunjay commented Aug 11, 2017 •

edited

Loading