Skip to content
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.

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

ci/test: improve CI jobs and tests #2189

Merged
merged 69 commits into from Mar 1, 2023
Merged

Conversation

DaniPopes
Copy link
Collaborator

@DaniPopes DaniPopes commented Feb 24, 2023

Motivation

Closes #1991
Closes #1978

Solution

TODO

Not planned for this PR (#2191):

PR Checklist

  • Added Tests
  • Added Documentation
  • Updated the changelog
  • Breaking changes

@DaniPopes DaniPopes marked this pull request as draft February 24, 2023 19:48
@DaniPopes DaniPopes marked this pull request as ready for review February 28, 2023 01:08
@DaniPopes
Copy link
Collaborator Author

DaniPopes commented Feb 28, 2023

@gakonst ready for review 🙏

Remaining failing actions:

  • deny (licenses): will open an issue later / tomorrow
  • windows: I have no clue what is wrong... It fails to compile a random example every time, which I cannot reproduce on my own machine, might be because of cache? Is there a way to clear that?
  • examples: fixed by fix: examples #2207


# Installs geth from https://geth.ethereum.org/downloads
install_geth() {
case "$PLATFORM" in
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it worth caching and then shortcutting if tool is already installed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, would need to check version in case they differ, but we're only downloading binary so it's not that important imo

use ethers_core::abi::{EventParam, Hash, ParamType};
use proc_macro2::Literal;

/// Expands a 256-bit `Hash` into a literal representation that can be used with
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this going somewhere? is dead code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't understand it; it's a local function in the test module, so it's not used anywhere else and had a test.. it doesn't really do anything

assert_eq!(Source::parse(abs).unwrap(), exp);
assert_eq!(Source::parse(abs_url).unwrap(), exp);
// Skip checking paths on Windows
#[cfg(not(windows))]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be re-written to use std::path instead of strings?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Realized that the diff was just the weird windows canonicalize thing, so dunce::canonicalize fixes that

@@ -1,951 +1,927 @@
#![allow(unused)]
use crate::common::*;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a big diff. I'm guessing git is over-reporting a file rearrangement? can you summarize changes?

Copy link
Collaborator Author

@DaniPopes DaniPopes Feb 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's 99% indentation, it contained only one module so I extracted that

#[tokio::test]
async fn contract_call_into_future_is_send() {
abigen!(DsProxyFactory, "ethers-middleware/contracts/DsProxyFactory.json");
fn _contract_call_into_future_is_send() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now unused?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a compilation test

// geth uses stderr for its logs
cmd.stderr(Stdio::piped());

let mut unused_ports = unused_ports::<3>().into_iter();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is good

Copy link
Owner

@gakonst gakonst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks great to me. Would love if we can double click on why any of the new tests are failing, and if we ultimately think it's OK I'm down to track in a new issue and @DaniPopes can handle the next steps?

chmod +x "solc$EXT"
}

main
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

@@ -1,14 +0,0 @@
name: Security audit
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want to keep this. Any reason not to?

Copy link
Collaborator Author

@DaniPopes DaniPopes Feb 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see deps.yml file, changed to use the one reth uses


#[test]
#[rustfmt::skip]
fn expand_hash_value() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was this unused?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -95,6 +95,7 @@ pub enum Eip2930RequestError {
pub struct Eip2930TransactionRequest {
#[serde(flatten)]
pub tx: TransactionRequest,
#[serde(rename = "accessList")]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yikes. @prestwich lol how was this never caught before, I guess nobody ever used direct 2930, and we do camelCase all in 1559 for access lists

@@ -427,7 +433,7 @@ impl Geth {
serde_json::to_writer_pretty(&mut file, &genesis)
.expect("could not write genesis to file");

let mut init_cmd = Command::new(GETH);
let mut init_cmd = Command::new(bin_path);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch

@@ -0,0 +1,76 @@
//! Etherscan integration tests
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love that we're leaning on thetests/it/main.rs pattern to save time compiling

use ethers_signers::{LocalWallet, Signer};

#[tokio::test]
#[ignore]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @prestwich who's worked on the gas escalators before.

What is the current issue you're facing?

ethers-middleware/tests/it/wallets.rs Outdated Show resolved Hide resolved
abigen!(SimpleStorage, "../tests/testdata/SimpleStorage.json");

#[tokio::test]
#[ignore]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/dapphub/ds-proxy we should understand why it's failing

- name: live tests
run: cargo test -p ethers --test live --all-features

# TODO: [#2191](https://github.com/gakonst/ethers-rs/issues/2191)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sgtm - we can do as followup

@gakonst
Copy link
Owner

gakonst commented Mar 1, 2023

OK. send it

@gakonst gakonst merged commit da743fc into gakonst:master Mar 1, 2023
@DaniPopes
Copy link
Collaborator Author

🫡

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move integration tests in 1 binary missing CI coverage
3 participants