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

appservice: Exclude from coverage CI for now #265

Conversation

johannescpk
Copy link
Contributor

@johannescpk johannescpk commented Jun 9, 2021

  • Coverage CI still enables the appservice feature, so I guess for appservice coverage that would need specific handling. Excluding for now
  • Also renames the autojoin example to get rid of cargo warning about duplicated example name

Part of #228

@johannescpk johannescpk force-pushed the appservice/ci/disable-coverage branch from ffac935 to e33bdb9 Compare June 9, 2021 21:14
@johannescpk johannescpk force-pushed the appservice/ci/disable-coverage branch from e33bdb9 to 248078e Compare June 9, 2021 21:17
@poljar
Copy link
Contributor

poljar commented Jun 10, 2021

Wouldn't disabling the failing test when the app service feature is enabled get us most of the way there?

diff --git a/matrix_sdk/src/client.rs b/matrix_sdk/src/client.rs
index 47227b473..41d073cb9 100644
--- a/matrix_sdk/src/client.rs
+++ b/matrix_sdk/src/client.rs
@@ -2706,7 +2706,6 @@ mod test {
             client::{
                 self as client_api,
                 r0::{
-                    account::register::{RegistrationKind, Request as RegistrationRequest},
                     directory::{
                         get_public_rooms,
                         get_public_rooms_filtered::{self, Request as PublicRoomsFilterRequest},
@@ -2714,7 +2713,7 @@ mod test {
                     media::get_content_thumbnail::Method,
                     membership::Invite3pidInit,
                     session::get_login_types::LoginType,
-                    uiaa::{AuthData, UiaaResponse},
+                    uiaa::AuthData,
                 },
             },
             error::{FromHttpResponseError, ServerError},
@@ -3042,7 +3041,13 @@ mod test {
     }
 
     #[tokio::test]
+    #[cfg(not(feature = "appservice"))]
     async fn register_error() {
+        use ruma::api::client::r0::{
+            account::register::{RegistrationKind, Request as RegistrationRequest},
+            uiaa::UiaaResponse
+        };
+
         let homeserver = Url::from_str(&mockito::server_url()).unwrap();
         let client = Client::new(homeserver).unwrap();

@johannescpk
Copy link
Contributor Author

While we could do that I'd say we want to avoid having the appservice feature active for regular non-appservice tests, since the modifications based on that feature will only increase and might lead to test results that don't match real world non-appservice usage

@johannescpk
Copy link
Contributor Author

Like, already now, all tests going through the send_request path would potentially take a different path than they would without the feature active

@poljar
Copy link
Contributor

poljar commented Jun 10, 2021

Yeah that's true, but that does mean that cargo test will start failing if the appservice feature gets turned on.

Perhaps we'll need to modify the feature to only enable the ability to become an app service but people would need to configure the Client object to modify it's behavior to be an app service. There's another upside to this, you'll be able to have app service style clients in your project at the same time you have a normal Client object around.

@johannescpk
Copy link
Contributor Author

I see, the issue also being dev experience where you can't just run cargo test in the root anymore, which isn't desirable indeed.

Deciding app service functionality at runtime feels less clean, but it'd indeed unlock having both in one project, which might not only be nice to have - but should also prevent things from failing unexpectedly, which currently could happen if both approaches are mixed in one project. I'll look into that. 👍

@johannescpk johannescpk marked this pull request as draft June 10, 2021 08:43
@poljar poljar closed this in #266 Jun 10, 2021
@johannescpk johannescpk deleted the appservice/ci/disable-coverage branch June 10, 2021 11:52
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