feat: test faucet website #702
Conversation
TomasArrachea
force-pushed
the
tomasarrachea-test-faucet-website
branch
from
dad20af to
801aa22
Compare
bin/faucet/src/main.rs
Outdated
| let mut chromedriver = Command::new("chromedriver") | ||
| .arg(format!("--port={chromedriver_port}")) | ||
| .spawn() | ||
| .expect("failed to start chromedriver"); |
There was a problem hiding this comment.
I had to install Chromedriver and Chrome to run this locally, but I am not sure on why it's running fine in the CI
There was a problem hiding this comment.
The github CI images have a bunch of pre-installed stuff (I believe the image is like 50GB). This means less cpu wasted installing things etc. And given the amount of webdev, probably almost any semi-popular tooling for it will be pre-installed.
There was a problem hiding this comment.
Should we create a target in the makefile to install Chromedriver and Chrome?
There was a problem hiding this comment.
I don't think you can unless you assume the OS? Its a pity the dep doesn't have a bundling option, makes this sort of thing much easier.
TomasArrachea
requested review from
igamigo,
tomyrd,
SantiagoPittella and
Mirko-von-Leipzig
There was a problem hiding this comment.
Looks good & interesting (not something I'm familiar with), thank you.
I've left some initial questions and suggestions; I think we may also want to organise the test files a bit somehow but we can address that after everything else.
Right now this tests that the website loads and has the correct title. Would we then extend this with requesting funds from the faucet as well? (separate PR etc maybe).
bin/faucet/src/main.rs
Outdated
| .ok_or(anyhow::anyhow!( | ||
| "Couldn't get any socket addrs for endpoint: {}", | ||
| config.endpoint | ||
| ))?; |
There was a problem hiding this comment.
| .ok_or(anyhow::anyhow!( | |
| "Couldn't get any socket addrs for endpoint: {}", | |
| config.endpoint | |
| ))?; | |
| .with_context(|| format!("no sockets available on {}, config.endpoint))?; |
There was a problem hiding this comment.
Though I'm unsure under what circumstances that would even occur.
bin/faucet/src/main.rs
Outdated
| .with_state(faucet_state) | ||
| } | ||
|
|
||
| async fn serve_faucet(config: FaucetConfig) -> anyhow::Result<()> { |
There was a problem hiding this comment.
Maybe this could go above create_router()
bin/faucet/src/main.rs
Outdated
| // Start the faucet connected to the stub | ||
| let config = FaucetConfig { | ||
| node_url: stub_node_url, | ||
| faucet_account_path: "src/test_data/faucet.mac".into(), |
There was a problem hiding this comment.
Should this file be outside /src?
bin/faucet/src/main.rs
Outdated
| // Wait for the server to be running | ||
| sleep(Duration::from_secs(1)).await; |
There was a problem hiding this comment.
Is this consistent? I guess 1 second doesn't hurt but otherwise maybe there could be some loop that retries connections a couple of times or something to make it a bit more robust.
There was a problem hiding this comment.
Removed this by reading stdout to wait until chromedriver is running: 102b761
bin/faucet/src/main.rs
Outdated
| // Start the faucet connected to the stub | ||
| let config = FaucetConfig { | ||
| node_url: stub_node_url, | ||
| faucet_account_path: "test_data/faucet.mac".into(), |
There was a problem hiding this comment.
Would it be possible to generate this as part of the test instead?
I mainly ask because I dislike random binary files floating around the codebase.
This does bring up an idea though - if we factor out the match &cli.command from main.rs into a separate function, then we could:
- Revert the
serve_faucetchanges (move them back toCommand::start. This isn't overly important, just a smaller diff. - Run the full faucet init, account creation and start commands in order.
This would also require using tempfs to host the files.
I rather like this idea. Though maybe it would be best to merge this PR and upgrade it in another PR.
@TomasArrachea WDYT? cc @igamigo
There was a problem hiding this comment.
Downside of this is that the test is now taking a bit longer to run, bit I think it's fine. It was a simple change so I just pushed the changes in this PR
There was a problem hiding this comment.
Approving, code LGTM. Merging will depend on whether we want to upgrade the test in this PR, or in a follow-up.
| let config_path = temp_dir().join(PathBuf::from("faucet.toml")); | ||
| let faucet_account_path = temp_dir().join(PathBuf::from("account.mac")); |
There was a problem hiding this comment.
From memory join takes Into<Path> or no?
| let config_path = temp_dir().join(PathBuf::from("faucet.toml")); | |
| let faucet_account_path = temp_dir().join(PathBuf::from("account.mac")); | |
| let config_path = temp_dir().join("faucet.toml"); | |
| let faucet_account_path = temp_dir().join("account.mac"); |
Closes #673
This PR adds a test for the faucet website. It uses
fantoccinias a browser client to test that all components are loading correctly.To run
fantocciniit needs a webdriver running. I'm using chromedriver here, which requires to have Chrome installed.Also, this PR introduces a stub for the node RPC API. This allows to run the faucet and test it using the stub, instead of having to run the full node.