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

Setup e2e tests #185

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Setup e2e tests #185

wants to merge 7 commits into from

Conversation

rmn-boiko
Copy link
Contributor

No description provided.

@rmn-boiko rmn-boiko self-assigned this Jan 23, 2025
@rmn-boiko rmn-boiko force-pushed the feat/add-e2e-tests branch 4 times, most recently from 168e611 to f12ee5a Compare January 24, 2025 14:31
@rmn-boiko rmn-boiko marked this pull request as ready for review January 24, 2025 15:06
@rmn-boiko rmn-boiko force-pushed the feat/add-e2e-tests branch 2 times, most recently from f2e3c6e to e8e0fb8 Compare February 4, 2025 19:09
@rmn-boiko rmn-boiko marked this pull request as draft February 4, 2025 19:54
@rmn-boiko rmn-boiko marked this pull request as ready for review February 4, 2025 19:54
Copy link
Member

@s373r s373r left a comment

Choose a reason for hiding this comment

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

Good step forward!

env:
SQLX_OFFLINE: false
DATABASE_URL: sqlite://kamu.sqlite.db
# platform(target) is a workaround for Windows only issue in cargo-nextest upon proc macro crates
Copy link
Member

Choose a reason for hiding this comment

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

We don't have Windows agents for kamu-node CI

Makefile Outdated
Comment on lines 66 to 67
$(KAMU_CONTAINER_RUNTIME_TYPE) stop kamu-postgres || true && $(KAMU_CONTAINER_RUNTIME_TYPE) rm kamu-postgres || true
$(foreach crate,$(POSTGRES_CRATES),rm $(crate)/.env -f ;)
Copy link
Member

Choose a reason for hiding this comment

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

Q: what if I work with two repos (kamu-cli & kamu-node), will they use the same DB (container)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, changed to unique one

@@ -942,20 +942,9 @@
]
}
},
"/odata/{account_name}": {
Copy link
Member

@s373r s373r Feb 5, 2025

Choose a reason for hiding this comment

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

Looks like a mistake, because we need an account

GET https://api.europort.kamu.dev/odata/acme.fishing.co

<?xml version="1.0" encoding="utf-8"?>
<service xml:base="https://api.europort.kamu.dev/odata"
	xmlns="http://www.w3.org/2007/app"
	xmlns:atom="http://www.w3.org/2005/Atom">
	<workspace>
		<atom:title>default</atom:title>
		<collection href="vessels.location-annotated">
			<atom:title>vessels.location-annotated</atom:title>
		</collection>
		<collection href="vessels.location-trawl">
			<atom:title>vessels.location-trawl</atom:title>
		</collection>
		<collection href="vessels.gps">
			<atom:title>vessels.gps</atom:title>
		</collection>
		<collection href="vessels.trawl">
			<atom:title>vessels.trawl</atom:title>
		</collection>
		<collection href="vessels.fuel">
			<atom:title>vessels.fuel</atom:title>
		</collection>
	</workspace>
</service>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

YEah, for some reason, it generetage wrong spec

Comment on lines 35 to 39
pub fn with_multi_tenant(mut self) -> Self {
self.is_multi_tenant = true;

self
}
Copy link
Member

Choose a reason for hiding this comment

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

in my opinion, we want to test kamu-node always in mt mode.

Q: in what case, do we benefit from st?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree, I actually did like this in the first time, but after I saw that there are chance to start it in st mode, but it will be much easier to keep it always mt

Self::new(options, Some(kamu_config))
}

pub fn mysql(mysql_pool: &MySqlPool, options: KamuNodeApiServerHarnessOptions) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

we don't have MySQL for E2E tests

token: Option<AccessToken>,
}

impl KamuApiServerClient {
Copy link
Member

Choose a reason for hiding this comment

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

General question: can we generalize (in trait) the methods of both clients? And in case of difference, have kamu-cli and kamu-node specific implementations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good suggestion, will add a follow up improvements to move this to separate crate in kamu-cli and reuse it here

S: AsRef<std::ffi::OsStr>,
T: Into<Vec<u8>> + Send;

async fn complete<T>(&self, input: T, current: usize) -> Vec<String>
Copy link
Member

Choose a reason for hiding this comment

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

I think some of the methods of this trait can be removed -- like this one, we don't plan to use autocomplete for kamu-node :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, doublechecked, and cleanedup some methods

Copy link
Member

Choose a reason for hiding this comment

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

those are definitely unneeded files (all *.sqlite)

@rmn-boiko rmn-boiko force-pushed the feat/add-e2e-tests branch 2 times, most recently from 8ecc739 to 8dc0cba Compare February 5, 2025 16:51
@rmn-boiko rmn-boiko requested a review from s373r February 5, 2025 17:38
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.

2 participants