10000 Resets the connection (and sets partial download to false) if the range is not satisfiable by the server by rberenguel · Pull Request #795 · coursier/coursier · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Resets the connection (and sets partial download to false) if the range is not satisfiable by the server #795

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Mar 12, 2018
Merged

Resets the connection (and sets partial download to false) if the range is not satisfiable by the server #795

merged 2 commits into from
Mar 12, 2018

Conversation

rberenguel
Copy link
Contributor
@rberenguel rberenguel commented Feb 26, 2018 •
edited
Loading

Addresses Issue #794

@rberenguel rberenguel mentioned this pull request Feb 26, 2018
Closed
@wisechengyi
Copy link
Collaborator

Looks good in general, but could you add a test to it?

@rberenguel
Copy link
Contributor Author

@wisechengyi thanks, will love to, but I’m not sure how/where to test this behaviour. I guess in tests/jvm/src/it/[...] would be the place to do it?

@alexarchambault
Copy link
Member

About testing that, there's already a small http server running during the integration tests (used here for example). It could be used, along with a cache from a temporary directory, to ensure that the cache download behaves well in that case.

The test can be added under cache/src/it/scala/… (which has to be created). You may need to add utest to the cache project, and enable integration tests for it (done for the tests project for example).

@alexarchambault
Copy link
Member

The test server can be manually launched via

$ scripts/launch-test-repo.sh --port 8080 --list-pages

@rberenguel
Copy link
Contributor Author

Thanks for the pointers @alexarchambault I'll get this tested ASAP

@rberenguel
Copy link
Contributor Author

@alexarchambault I think I need another pointer, can't figure out a crucial step here. I'm using

coursier/tests/jvm/src/it/scala/coursier/test/HttpAuthenticationTests.scala

Lines 56 to 59 in 2747d1e

MavenRepository(
s"http://$address",
authentication = Some(Authentication(user, password))
)
as a model (well, I'm not using check but a custom method because I need to mess with the cache). But I can't figure out how to get artifacts to appear in the cache temporary directory (I'm basing off check here: https://github.com/coursier/coursier/blob/master/tests/jvm/src/test/scala/coursier/test/CacheFetchTests.scala ) After the resolution step only poms, shas and similar appear in the cache folder, even if the mock server contains jar artifacts for the module. My impression was that once the Resolution finishes the cache should be populated, but that might not be the case?

@alexarchambault
Copy link
Member

After the resolution, only the POMs (and their checksums) are fetched. JARs are fetched in a second time (it corresponds to the Task.gatherUnordered in the API example of the README).

To test your changes, I think you can just create a .part file in the cache, corresponding to a file of the http server of the integration tests (like copy a file from tests/jvm/src/test/resources/test-repo/http/abc.com/com/… to the cache under http/localhost%3A8080/com/… and rename it to a .part file). Then you can make an Artifact fetching the corresponding URL (http://localhost:8080/com/…), and ask Cache.fetch or Cache.file to fetch it, and see if it succeeds.

@alexarchambault
Copy link
Member

I'd be ok with merging as is without proper tests, as the cache module itself doesn't have a test suite. We can just add a comment saying loudly what the added condtion handles, and that it can be tested once cache has some proper tests… On my side, I was planning to add some while porting the cache module to nio. Tests for this can be added after that IMO.

@rberenguel
Copy link
Contributor Author

Hi @alexarchambault I'm a bit busy atm, but I'd feel bad merging without the test in place now I have a vague idea of how to do it. Alternatively, it could be merged and I create a new issue (795 has no tests, or cache module has no tests) and then I work on that issue

@wisechengyi
Copy link
Collaborator

How about doing so from the cli test suite side? like https://github.com/coursier/coursier/pull/797/files#diff-091ffa792cee5668b5b2ab31820ac6eeR756, but in this case we are not corrupting the jar, but just rename it to xxx.jar.part. and it does not involve setting up a http server.

@rberenguel
Copy link
Contributor Author

Hi @wisechengyi @alexarchambault thanks a lot, indeed. I worked off your PR (to have the full CliFetchIntegrationTest) and the following test validates this PR (I changed the junit version to help me while reading logs):

  "Wrong range partial artifact resolve" should "succeed with retry" in withTempDir("tmp_dir") {
    dir => {
      def runFetchJunit = {
        val fetchOpt = FetchOptions(common = CommonOptions(mode = "force", cacheOptions = CacheOptions(cache = dir.getAbsolutePath)))
        val fetch = Fetch(fetchOpt, RemainingArgs(Seq("junit:junit:4.6"), Seq()))
        assert(fetch.files0.map(_.getName).toSet
          .equals(Set("junit-4.6.jar")))
        val junitJarPath = fetch.files0.map(_.getAbsolutePath()).filter(_.contains("junit-4.6.jar"))
          .head
        val junitJarFile = new File(junitJarPath)
        junitJarFile
      }

      val originalJunitJar: File = runFetchJunit

      val originalJunitJarContent = FileUtil.readAllBytes(originalJunitJar)

      // Move the jar to partial (but complete) download
      val newJunitJar: File = new File(originalJunitJar.getAbsolutePath + ".part")
      originalJunitJar.renameTo(newJunitJar)

      // Run fetch again and it should pass because of retrying on the partial jar.
      val jar = runFetchJunit
      assert(FileUtil.readAllBytes(jar).sameElements(originalJunitJarContent))
    }
  }

@rberenguel
Copy link
Contributor Author

@alexarchambault if PR #799 is merged then I can add my test to the rest of the CLI suite

@rberenguel
Copy link
Contributor Author

@wisechengyi @alexarchambault test added, thanks for all the help to get it :)

@alexarchambault
Copy link
Member

Thanks @rberenguel!

@alexarchambault alexarchambault merged commit cf365ea into coursier:master Mar 12, 2018
@NightMachinery NightMachinery mentioned this pull request Jun 1, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wisechengyi wisechengyi wisechengyi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rberenguel @wisechengyi @alexarchambault
0