Use random username in tests #154

anuragsoni · 2017-01-08T19:37:29Z

This is a first pass at using randomly generated user id's in tests

closes #121

anuragsoni · 2017-01-08T19:38:54Z

@mujx @jimmycuadra I haven't changed all occurrences of user_id's in tests yet. Updates will be required in all tests and I figured i'd do that if the approach that i've take here is agreeable to you guys.

mujx

Thanks a lot for your work.

Looks good to me 👍 Just a few minor details.

mujx · 2017-01-08T19:57:55Z

src/test.rs

+/// Used to return the randomly generated user id and access token
+#[derive(Debug)]
+pub struct UserToken {
+    pub user_id: String,


You can use UserId here and maybe rename it to id so we can use alice.id which is a little more natural than alice.user_id.

I think i'd prefer id

mujx · 2017-01-08T20:00:50Z

src/test.rs


 static START: Once = ONCE_INIT;

 const DATABASE_URL: &'static str = "postgres://postgres:test@postgres:5432/ruma_test";
 const POSTGRES_URL: &'static str = "postgres://postgres:test@postgres:5432";

+/// Used to return the randomly generated user id and access token
+#[derive(Debug)]
+pub struct UserToken {


I think a more appropriate name here is TestUser, User or something like that because it doesn't include only the access token.

That seems reasonable. TestUser will indeed be a better fit, and is flexible enough we something more is added in the future

mujx · 2017-01-08T20:03:25Z

src/test.rs

@@ -164,6 +184,7 @@ impl Test {
    }

    /// Registers a new user account with the given username and returns the user's access token.
+    #[deprecated(note="please use `create_access_token_with_random_id` instead")]
    pub fn create_access_token_with_username(&self, username: &str) -> String {


You can remove this though when you convert all the tests. It's just a helper method and no api will break.

mujx · 2017-01-08T20:08:20Z

src/test.rs

@@ -174,6 +195,20 @@ impl Test {
            .to_string()
    }

+    /// Registers a new user account with a random user id of given length and returns
+    /// the user's id and token
+    pub fn create_access_token_with_random_id(&self, len: usize) -> UserToken {


You can remove the len attribute and use something constant, like 10 or generate a random one within a range.
Maybe rename the method to something like create_user or register_new_user etc; it describes a little better what this function does and you can hide the implementation details (random id).

mujx · 2017-01-08T20:16:59Z

src/api/r0/event_creation.rs

@@ -538,43 +539,41 @@ mod tests {
    #[test]
    fn send_message_without_room_membership() {
        let test = Test::new();
-        let bob_token = test.create_access_token_with_username("bob");
-        let alice_token = test.create_access_token_with_username("alice");
+        let UserToken{ user_id: alice, token: alice_token} = test.create_access_token_with_random_id(5);


I would prefer the

let alice = test.create_user(); alice.id, alice.token

or something similar which isn't too verbose. It will create a bigger diff though but that's not really a problem.

mujx · 2017-01-08T20:18:12Z

src/api/r0/event_creation.rs

-            },
-            "users_default": 0
-        }"#;
+        let event_content = format!(r#"{{"ban": 100,"events": {{ "m.room.message": 100 }},"events_default": 0,"invite": 100,"kick": 100,"redact": 0,"state_default": 0,"users": {{"{}": 50}},"users_default": 0}}"#,


The line is too long. Keep the old style which is more readable.

I wasn't able to figure out how to apply a format! on a raw string. The other option I have is to escape the string and then I can split it on multiple lines as before

It works if you do

let event_contet = format!( r#"{{ "ban": 100, ... "users_default": 0 }}"#, bob);

or something similar.

mujx · 2017-01-08T20:23:24Z

src/test.rs

+    pub fn new(username: String, token: String) -> Self {
+        UserToken { user_id: format!("@{}:ruma.test", username), token: token }
+    }
+


Empty line.

anuragsoni · 2017-01-08T23:49:47Z

updated by removing create_access_token, create_access_token_with_username, and by returning the new user struct (by using the new create_user method) in create_fixtures instead of returning just a token.

mujx

Just formatting issues.

mujx · 2017-01-09T00:15:26Z

src/test.rs

-            .unwrap()
-            .to_string()
-    }
-
    /// Registers a new user account with a random user id of given length and returns


Update the description here.

mujx · 2017-01-09T00:23:31Z

src/api/r0/event_creation.rs

        );

-        let event_content = format!(r#"{{"ban": 100,"events": {{ "m.room.message": 100 }},"events_default": 0,"invite": 100,"kick": 100,"redact": 0,"state_default": 0,"users": {{"{}": 50}},"users_default": 0}}"#,
-                                    bob);
+        let event_content = format!(r#"{{


Try writing these like the ones from r0/room_creation.rs.

mujx · 2017-01-09T00:23:43Z

src/api/r0/event_creation.rs

        assert_eq!(response.status, Status::Forbidden);
        assert_eq!(
            response.json().find("error").unwrap().as_str().unwrap(),
            "Insufficient power level to create this event."
        );

-        let event_content = format!(r#"{{"ban": 100,"events": {{"m.room.message": 0 }},"events_default": 0,"invite": 100,"kick": 100,"redact": 0,"state_default": 0,"users": {{"{}": 50}},"users_default": 0}}"#, bob);
+        let event_content = format!(r#"{{


Same with above.

mujx · 2017-01-09T00:34:39Z

src/api/r0/room_creation.rs


-        let room_options = r#"{"visibility": "private",
+        let room_options = format!(r#"{{"visibility": "private",


Indent this like the example below.

format!(r#"{{ "visibility": "private", ... }}"#, bob.id, carl.id);

mujx · 2017-01-09T00:36:53Z

src/test.rs

    pub token: String,
+    pub name: String,


👍 for username.

mujx · 2017-01-09T00:38:59Z

src/api/r0/join.rs

-            r#"{"visibility": "private", "invite": ["@alice:ruma.test"]}"#
-        );
+            &bob.token,
+            format!(


Use one line for format!.

mujx · 2017-01-09T00:39:16Z

src/api/r0/join.rs

-            r#"{"visibility": "private", "invite": ["@alice:ruma.test"]}"#
-        );
+            &bob.token,
+            format!(


Use one line for format!.

mujx · 2017-01-12T14:51:34Z

Turns out there is already a random id generator in ruma-identifiers so we can use that. It is used in registration if a username isn't provided. The other option is to omit the username field and get a random one back.

anuragsoni · 2017-01-12T15:01:41Z

@mujx Ah, I hadn't seen ruma-identifiers. It looks to be the same thing that I had done in this PR so I replace my function with a call to the one from ruma-identifiers. As for the username, I find it nicer to have in the TestUser. I was generating the username randomly, but there are tests where we need both the username and the id. This field helps for that.

mujx · 2017-01-12T15:07:09Z

Yes we need to keep it inside TestUser. The registration endpoint will return a random username. So you can do json().find('username') to get it, if you want. Here is the response.

anuragsoni · 2017-01-12T15:10:54Z

that returns only the user_id?

#[derive(Debug, Serialize)]
struct RegistrationResponse {
    pub access_token: String,
    pub home_server: String,
    pub user_id: String,
}

mujx · 2017-01-12T15:13:28Z

That will return the fully qualified Matrix ID, ie @zikuoafjyyrh:ruma.test

anuragsoni · 2017-01-12T15:17:51Z

exactly. My point being that we need the id => @zikuoafjyyrh:ruma.test and the username: zikuoafjyyrh. If I register without the username, I'd need to access the random username that ruma_identifier creates. But I do not see it being returned in the RegistrationResponse struct? Maybe i'm missing something?

mujx · 2017-01-12T15:24:10Z

There is a helper method for UserId to get the localpart that you are reffering to. You can parse the returned UserId and call user_id.localpart() to the get the username you want. Another way to go is to store the id as UserId instead of String in the TestUser and call alice.localpart() in the tests.

This reverts commit e60e0e8.

jimmycuadra · 2017-01-14T01:25:58Z

This should be easier now with #158 merged!

anuragsoni · 2017-01-14T02:13:02Z

Updated to use the UserId from ruma_identifier, and squashed commits for cleanup

jimmycuadra · 2017-01-14T02:52:49Z

One minor thing to fix that I just noticed: We organize imports into three sections, separated by blank lines: 1) stdlib imports 2) external crate imports 3) internal imports. The two imports added to src/test.rs should be resorted accordingly. Go ahead and squash the commits again after that and I'll merge this. Thanks very much for doing this work—that was a lot of little things to change!

anuragsoni · 2017-01-14T02:57:18Z

@jimmycuadra I will keep that in mind for any change I work on in future!
Updated with the imports re-ordered.

jimmycuadra · 2017-01-14T03:05:41Z

We'll probably add some minor style guide information to CONTRIBUTING.md soon, and eventually rustfmt will fix all of this nonsense.

mujx suggested changes Jan 8, 2017

View reviewed changes

mujx added component/tests status/work in progress type/improvement labels Jan 8, 2017

anuragsoni changed the title ~~Use random username in tests~~ Use random username in tests. Closes #121 Jan 8, 2017

anuragsoni changed the title ~~Use random username in tests. Closes #121~~ Use random username in tests Jan 8, 2017

mujx suggested changes Jan 9, 2017

View reviewed changes

mujx approved these changes Jan 9, 2017

View reviewed changes

anuragsoni mentioned this pull request Jan 12, 2017

Use ruma identifiers instead of Strings #158

Merged

Revert "Locks on objects should follow consistent order"

1208624

This reverts commit e60e0e8.

Generate Random Users for tests

2191baa

jimmycuadra merged commit acd52c8 into ruma:master Jan 14, 2017


		let room_options = r#"{"visibility": "private",
		let room_options = format!(r#"{{"visibility": "private",

Use random username in tests #154

Use random username in tests #154

Conversation

anuragsoni commented Jan 8, 2017 • edited Loading

anuragsoni commented Jan 8, 2017

mujx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuragsoni commented Jan 8, 2017

mujx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mujx commented Jan 12, 2017

anuragsoni commented Jan 12, 2017

mujx commented Jan 12, 2017 • edited Loading

anuragsoni commented Jan 12, 2017

mujx commented Jan 12, 2017

anuragsoni commented Jan 12, 2017

mujx commented Jan 12, 2017

jimmycuadra commented Jan 14, 2017

anuragsoni commented Jan 14, 2017

jimmycuadra commented Jan 14, 2017

anuragsoni commented Jan 14, 2017

jimmycuadra commented Jan 14, 2017

anuragsoni commented Jan 8, 2017 •

edited

Loading

mujx commented Jan 12, 2017 •

edited

Loading