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

NTRN-195 feat: relayer keyring tests #31

Draft
wants to merge 33 commits into
base: main
Choose a base branch
from
Draft

Conversation

oldremez
Copy link
Contributor

@oldremez oldremez commented Oct 31, 2022

This PR contains the extension of the test framework with tests of the query-relayer configurations with different keyrings (pull request reflecting these changes in query-relayer: neutron-org/neutron-query-relayer#53).

The different configurations of query-relayer are managed by wrapper containers. The default image has a version "base" (see .nev near docker-compose.yml) and different configurations have appropriate version names. The version being provided to the test launching command forces docker-compose to take an appropriate version of the image.

The main drawback is that we should be more careful with launching the query-relayer without specifying the version (the latest one might have some unpredictable configuration). To avoid it we can differentiate those images by their name and not by version.

The other drawback is that keyring tests don't work in a non-docker environment.

Notions:

  1. kwallet isn't tested. The reasons are: it's not something that is used by people and it's UI-only thing. The ways I see it could be tested (both are hugely unreasonable, so I left it as is):
    1. Launch some in-memory X server to fake wallet somehow
    2. Mock kwallet itself on the d-bus level
  2. Keyring tests are really time-consuming: it takes ~400s on my machine. Maybe we should separate 'quick' and 'full' tests or something not to bother everyone with a long testing time.
  3. We should think about separating the environment setup and tests. It would be much easier for me if I could launch tests on an arbitrary environment instead of struggling with mixed setup/testing logic (so, the proposal is to move the testing environment configuration out of jest to higher level)

env: jestEnvironment,
};

const jestPromise = exec(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just an idea, what if we just have interchain_kv_query test in package.json with postfixes equal to backends and defined RELAYER_VERSION for each run? (I hope someday we have all these tests running in CI).

Copy link
Contributor Author

@oldremez oldremez Nov 7, 2022

Choose a reason for hiding this comment

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

Of course, I considered this option during implementation, but it has several important (in my opinion) cons:

  1. Besides configuring the environment options, the test contains very feature-specific logic of watching in logs which backend is actually used by the relayer (lookForKeyringInContainerLogs function). This logic should be implemented in the separate test scenario code, not in the interchain_kv_query or any wrapper around it. Without checking the actual keyring in logs there is a chance for test to become false-positive because of refactoring or something (configuration will break and all the test will pass because of using test keyring for example).
  2. It breaks the flow of running all the tests. In my current implementation running yarn:test runs all the tests, including kering-related. In the proposed solution we will need some wrapper (shell script?) that will run yarn:test, RELAYER_VERSION=os yarn:interchain_kv_query, and so on. It gives a bigger chance for keyring tests to become lost or something. I tried to avoid adding additional layers not to increase the complexity.
  3. As I mentioned in the PR description, the source of the issue is the mixed logic of setting up the environment and testing. Eventually we should separate those; so, some wrapper should set up the environment and run the tests on it. Your proposal is a step in that direction but it leads to some sort of inconsistency (some tests are run this way and some tests are run the other way). Choosing between inconsistent shitty code and consistent shitty code I chose the last.
  4. The keyring test doesn't need to depend exactly on interchain_kv_query test. It just needs any test that makes transactions to Neutron, so interchain_kv_query may be replaced with any other test, e.g. less time-consuming one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please tell me if I got your idea wrong 🙃

Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. I'm not sure that I really got the idea behind constant testing selection of keyring type. I mean once you have reached it working it can't be broken. It looks overcomplicated.
  2. Agree
  3. Agree
  4. So this way I guess a bit better option would be having some simple action in relayer_keyring.test.ts itself bc calling one test from another looks not right

Copy link
Contributor Author

Choose a reason for hiding this comment

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

1: Let's assume that logs aren't checked. Let's also assume somebody accidentally removed the line jestEnvironment['RELAYER_VERSION'] = relayerVersion; in the test's code or replaced image: "neutron-org/neutron-query-relayer:${RELAYER_VERSION}" to image: "neutron-org/neutron-query-relayer:base" in the Dockerfile or any other mistake of this kind. In these cases, the tests will falsely pass, and nobody notices (the only way to detect it is code review which is not 100% guaranteed).
4: Yeah, I also don't love it a lot, agree that it maybe should be replaced with something more simple… Still, the current design allows doing so in a very simple manner, while the proposed solution in your first message doesn't (if I got it right). The thing I'm afraid of is that it will contain a lot of copy-paste from the interchain_kv_query/interchain_tx_query tests. But yeah, it might be better than launching test from test.

Copy link
Collaborator

@ratik ratik Nov 7, 2022

Choose a reason for hiding this comment

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

1: ok, got it. sounds reasonable 👍
4: So if there is much of c/p we can move some action parts into wrapper/helpers/classes/etc and reuse from the mentioned tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

4: Actually, I thought about leaving it as technical debt and resolving it in a separate task/PR… It would touch the code that is really out-of-scope of this task which I really scared about (I struggle with the integration tests even while adding the new one, modifying them in the context of this task sounds really unattractive). But maybe merging this test into main in its current form is worse, I dunno.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it. Agree. All-in-all the thing looks really impressive 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nah, I anyway reorganized the tests because it was a mess

@oldremez oldremez marked this pull request as draft December 8, 2022 00:09
@oldremez oldremez changed the title feat: relayer keychain tests NTRN-195 feat: relayer keychain tests Dec 9, 2022
@oldremez oldremez marked this pull request as ready for review December 9, 2022 22:29
@oldremez oldremez changed the title NTRN-195 feat: relayer keychain tests NTRN-195 feat: relayer keyring tests Dec 9, 2022
import { getRemoteHeight } from '../helpers/wait';

const lookForKeyringInContainerLogs = async (
realayerImageName: string,
Copy link
Contributor

Choose a reason for hiding this comment

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

realayerImageName — misprint here and in several other places nearby

@oldremez oldremez marked this pull request as draft April 6, 2023 10:01
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.

3 participants