Add a test to check the download of the latest version #648

r0qs · 2022-08-07T20:09:01Z

This PR attempt to solve the issue #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 · 2022-08-07T20:16:12Z

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

cameel · 2022-08-08T08:08:13Z

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 · 2022-08-08T09:13:58Z

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 · 2022-08-10T16:02:43Z

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 · 2022-08-13T11:07:04Z

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 · 2022-08-17T15:06:21Z

downloader.ts

+function getVersionList () {
+  console.log('Retrieving available version list...');
+
+  return new Promise<string>((resolve, reject) => {


Any reason to directly use promises rather than async/await here?

Yes. As far as I know the https.getmodule from nodejs is not "awaitable" so I needed to wrap it in a promise and explicitly resolve the response.

cameel · 2022-08-17T15:13:02Z

downloader.ts

+  return new Promise<string>((resolve, reject) => {
+    const mem = new MemoryStream(null, { readable: false });
+    https.get('https://binaries.soliditylang.org/bin/list.json', function (response) {
+      if (response.statusCode !== 200) {


It's something that has already been here before but we should change this so that anything between 200 and 299 is a success. I think we also have the same check in other places in the code

But I'd do it in a separate PR since it's a functional change, not just a refactor and is also unrelated to tests.

response.ok?

I don't think the https nodejs module has such ok property. I know that the response object of the fecth API has it, but it is not what we are using here. However, we could mimic its behavior.

Ah, you're right, I was actually looking at the fetch API when I made the comment.

downloadCurrentVersion.ts

test/downloader.ts

cameel · 2022-08-17T15:55:47Z

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 · 2022-09-17T09:26:15Z

️✅ 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.

^{_{🦉 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!}}

r0qs · 2022-09-17T14:52:59Z

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 #659 for the CI bash integration test.

matheusaaguiar

I am actually learning JS/TS more than anything with this review, but here's my 2 cents :)

downloadCurrentVersion.ts

downloader.ts

test/downloader.ts

Co-authored-by: matheusaaguiar <[email protected]>

downloadCurrentVersion.ts

downloader.ts

test/downloader.ts

downloadCurrentVersion.ts

Co-authored-by: matheusaaguiar <[email protected]>

matheusaaguiar

LGTM.

cameel requested a review from nikola-matic August 10, 2022 13:31

cameel reviewed Aug 17, 2022

View reviewed changes

r0qs mentioned this pull request Sep 17, 2022

Add CI job to check if updateBinary downloads the correct release #659

Open

cameel requested a review from matheusaaguiar September 19, 2022 15:53

matheusaaguiar reviewed Sep 20, 2022

View reviewed changes

downloadCurrentVersion.ts Outdated Show resolved Hide resolved

downloader.ts Outdated Show resolved Hide resolved

test/downloader.ts Outdated Show resolved Hide resolved

test/downloader.ts Outdated Show resolved Hide resolved

r0qs and others added 8 commits October 31, 2022 14:53

Promisify solc-js downloader

78997dd

Test solc-js download

5231691

Add download unit tests

5380321

Minor adjusts

f920076

Fix host url

8b4c2e5

Update downloader.ts

e054e70

Co-authored-by: matheusaaguiar <[email protected]>

Print error message in test failure assertion

515ec2b

Move hashFile function to common place

ad40b4c

r0qs force-pushed the test-download-latest-version branch from c24005f to ad40b4c Compare October 31, 2022 17:53

r0qs requested review from cameel and matheusaaguiar October 31, 2022 17:53

matheusaaguiar reviewed Nov 1, 2022

View reviewed changes

downloadCurrentVersion.ts Outdated Show resolved Hide resolved

downloader.ts Outdated Show resolved Hide resolved

test/downloader.ts Outdated Show resolved Hide resolved

test/downloader.ts Outdated Show resolved Hide resolved

nikola-matic reviewed Nov 1, 2022

View reviewed changes

downloadCurrentVersion.ts Outdated Show resolved Hide resolved

r0qs and others added 4 commits November 2, 2022 09:09

Update downloadCurrentVersion.ts

5893e2e

Co-authored-by: matheusaaguiar <[email protected]>

Update downloader.ts

26a0167

Co-authored-by: matheusaaguiar <[email protected]>

Update test/downloader.ts

461fa71

Co-authored-by: matheusaaguiar <[email protected]>

Replace filter by find

6e7f694

Change exception message when expected mock requests are not performed

1b16bb9

matheusaaguiar approved these changes Nov 2, 2022

View reviewed changes

r0qs requested a review from nikola-matic November 21, 2022 11:13

r0qs self-assigned this Mar 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a test to check the download of the latest version #648

Add a test to check the download of the latest version #648

r0qs commented Aug 7, 2022

coveralls commented Aug 7, 2022

cameel commented Aug 8, 2022

r0qs commented Aug 8, 2022 •

edited

Loading

cameel commented Aug 10, 2022

r0qs commented Aug 13, 2022 •

edited

Loading

cameel Aug 17, 2022

r0qs Sep 5, 2022 •

edited

Loading

cameel Aug 17, 2022

nikola-matic Nov 1, 2022

r0qs Nov 4, 2022 •

edited

Loading

nikola-matic Nov 4, 2022

cameel commented Aug 17, 2022

gitguardian bot commented Sep 17, 2022 •

edited

Loading

r0qs commented Sep 17, 2022

matheusaaguiar left a comment

matheusaaguiar left a comment

Add a test to check the download of the latest version #648

Are you sure you want to change the base?

Add a test to check the download of the latest version #648

Conversation

r0qs commented Aug 7, 2022

coveralls commented Aug 7, 2022

cameel commented Aug 8, 2022

r0qs commented Aug 8, 2022 • edited Loading

cameel commented Aug 10, 2022

r0qs commented Aug 13, 2022 • edited Loading

cameel Aug 17, 2022

Choose a reason for hiding this comment

r0qs Sep 5, 2022 • edited Loading

Choose a reason for hiding this comment

cameel Aug 17, 2022

Choose a reason for hiding this comment

nikola-matic Nov 1, 2022

Choose a reason for hiding this comment

r0qs Nov 4, 2022 • edited Loading

Choose a reason for hiding this comment

nikola-matic Nov 4, 2022

Choose a reason for hiding this comment

cameel commented Aug 17, 2022

gitguardian bot commented Sep 17, 2022 • edited Loading

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

r0qs commented Sep 17, 2022

matheusaaguiar left a comment

Choose a reason for hiding this comment

matheusaaguiar left a comment

Choose a reason for hiding this comment

r0qs commented Aug 8, 2022 •

edited

Loading

r0qs commented Aug 13, 2022 •

edited

Loading

r0qs Sep 5, 2022 •

edited

Loading

r0qs Nov 4, 2022 •

edited

Loading

gitguardian bot commented Sep 17, 2022 •

edited

Loading