Add a test to check the download of the latest version

[r0qs]

This PR attempt to solve the issue https://github.com/ethereum/solc-js/issues/632. It adds a test to verify if the current version is equal to the downloaded one, and it also modifies the downloader script to enable its use in the tests.

[coveralls]

Coverage decreased (-0.6%) to 83.099% when pulling 4d5ccb668e3772a8b3297344893cbdd11cf1e30a on r0qs:test-download-latest-version into c80296459dd068baceda57c0cfd08b262df5d04e on ethereum:master.

[cameel]

Thanks for the PR!

I'll take a closer look at it later but for now just wanted to say that solidity-solcjs-ext-test job is failing so there must still be something wrong there.

Also, the coverage bot says the coverage decreased, which means you need to add some more tests.

[r0qs]

The coverage decrease seems to be because I separated the downloadCurrentVersion.ts into two files, exported getVersionList and downloadBinary to be used on the test, but didn't add tests in case the download fails (e.g., HTTP 404). I believe these were not covered before either because the original code only logs and exits on error.

But I can add tests for it. However, maybe it would be better to mock the HTTP requests in those methods and pass the endpoint URL as parameter. What do you think?

About the CI, the problem is that I'm getting the latest version based on the version field in the package.json. But the CI updates the version to one patch version ahead, which was not released yet, so the test fails:

/tmp/ext-test-solc-js-nIDFB9
Using compiler version 0.8.16-ci.2022.8.5+commit.49a2db99.Emscripten.clang
Updating index.js file...
Copying determinism.js...
Copying contracts...
Copying SMTChecker tests...
Updating package.json to version 0.8.16
v0.8.16
Replacing fixed-version pragmas...
Running test function...
...

not ok 1120 Version 0.8.16 is not the latest release 0.8.15

...

Maybe I misunderstood the goal of the task. Should I not use the latest version in the package.json when trying to download? And instead, should I only test if the latest available version in the endpoint is the one that got downloaded?

[cameel]

But I can add tests for it. However, maybe it would be better to mock the HTTP requests in those methods and pass the endpoint URL as parameter. What do you think?

Yeah, definitely. I actually think that in the JS tests we would be better off with mocks both for positive and negative case. I'd consider an actual download an integration test and for that we might just go all the way - i.e. have a CI job that executes the script and does the check in Bash rather than trying to do it in JS. But, well, I'm not against having that part in JS - it's not like other tests here are pure unit tests :) It's a bit of a mess so whatever works is fine.

BTW, would mocking require adding some dependencies? We overall prefer to avoid too many deps - we have enough already and we should actually trim that even more.

Maybe I misunderstood the goal of the task. Should I not use the latest version in the package.json when trying to download?

Are you sure you're really getting the latest release version? 0.8.16-ci.2022.8.5+commit.49a2db99.Emscripten.clang is not a release version. It's a build from CircleCI. Not sure why you're actually getting this - https://binaries.soliditylang.org/bin/ (https://github.com/ethereum/solc-bin/tree/gh-pages/bin) has nightlies too and you might be getting that but then you'd have nightly in the version string, not ci.

[r0qs]

Yeah, definitely. I actually think that in the JS tests we would be better off with mocks both for positive and negative case. I'd consider an actual download an integration test and for that we might just go all the way - i.e. have a CI job that executes the script and does the check in Bash rather than trying to do it in JS. But, well, I'm not against having that part in JS - it's not like other tests here are pure unit tests :) It's a bit of a mess so whatever works is fine.

Great. I will do that then. Mocking the JS tests and adding the integration test in the CI.

BTW, would mocking require adding some dependencies? We overall prefer to avoid too many deps - we have enough already and we should actually trim that even more.

No, not necessarily. We can use the native HTTPS module of Node.js. So no new dependencies are needed :-)

Are you sure you're really getting the latest release version? 0.8.16-ci.2022.8.5+commit.49a2db99.Emscripten.clang is not a release version. It's a build from CircleCI. Not sure why you're actually getting this - https://binaries.soliditylang.org/bin/ (https://github.com/ethereum/solc-bin/tree/gh-pages/bin) has nightlies too and you might be getting that but then you'd have nightly in the version string, not ci.

I'm getting the current version from the package.json. See this line. And the released version is fetched from the "releases" entry in the list.json in this line. The problem was that version 0.8.16 was set in the package.json by the CI but did not exist in the list.json as a release.

[cameel]

I'm getting the current version from the package.json. See this line. And the released version is fetched from the "releases" entry in the list.json in this line. The problem was that version 0.8.16 was set in the package.json by the CI but did not exist in the list.json as a release.

Oh, you're right. Looks like the external test for solc-js that we have in the main repo actually bumps version in package.json. It's actually that test that's getting a binary from CI - and it's because it's supposed to test that the latest compiler binary works with solc-js. It's bumping version because solc-js and the compiler have different conventions for updating the version. In the main repo, we bump the version hard-coded in the main CMakeLists.txt immediately after a release while in solc-js we bump version in package.json just before a release. This means that version in solc-js is one number behind solc for most of the development cycle.

In any case, this just shows that this must be tested from the outside. I.e. we should not be assuming inside these JS tests that version in package.json is the most recent one. Instead this should be a Bash integration that installs solc-js on its own (so it can ensure that package.json was not changed), invokes downloadCurrentVersion.ts and makes sure it downloaded the right thing.

[gitguardian[bot]]

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them. Once a secret has been leaked into a git repository, you should consider it compromised, even if it was deleted immediately. Find here more information about risks.

---

<sup><sub>🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!</sub></sup>

[r0qs]

I added the unit tests for downloadBinary and getVersionList. I decided to use a dependency called nock to easily mock the HTTPS requests. I also added a dummy certificate that is only used in the tests, otherwise we would need to rewrite the downloadBinary and getVersionList to not use the HTTPS module directly but also support HTTP.

I opened another PR https://github.com/ethereum/solc-js/pull/659 for the CI bash integration test.