Added unit test to usage metrics endpoints #1133

jfkonecn · 2024-07-09T13:45:33Z

I created a private MockS3Data class with a files property which generates and stores the mock JSON S3 files and the expected responses from all the endpoints. I hard coded a seed for the random number generator so that the tests always have the same data passed to them which should make it easier to debug any breaks.

I made the class private since it seemed unnecessary to create file for a class that is only needed once.

I think I may have caught some bugs with some endpoints, but I opted to maintain the same behavior so the current expected response data may not look quite right. My goal is to make it obvious what the behavior changes are when I make the 3 year timeline change in UsageAnalysisController.

zhx828

Not completed done with my review but like to hear from you on my comments before moving on further. Thanks!

zhx828 · 2024-07-18T01:51:34Z

src/main/java/org/mskcc/cbio/oncokb/config/Constants.java

@@ -35,6 +35,7 @@ public final class Constants {

    public static final String MONTH_USERS_USAGE_SUMMARY_FILE_PREFIX = "public-website/usage-analysis/month-user-summary_";
    public static final String YEAR_USERS_USAGE_SUMMARY_FILE_PREFIX = "public-website/usage-analysis/year-user-summary_";
+    public static final String MONTH_RESOURCES_USAGE_SUMMARY_FILE_PREFIX = "public-website/usage-analysis/year-resource-summary_";


Do you mean to use month-resource-summary_?

zhx828 · 2024-07-18T02:32:47Z

src/test/java/org/mskcc/cbio/oncokb/web/rest/UsageAnalysisControllerIT.java

+      public final X first;
+      public final Y second;


Give some meaningful names here? It's used by this test anyway, may as well just use lastKey and jsonObject?

It's use by the logic that creates the mock json file, but I'll change it to KeyValuePair

src/test/java/org/mskcc/cbio/oncokb/web/rest/UsageAnalysisControllerIT.java

zhx828 · 2024-07-18T02:34:16Z

src/test/java/org/mskcc/cbio/oncokb/web/rest/UsageAnalysisControllerIT.java

+    }
+
+    private User createMockUser(
+      int userIndex,


Nit picking, should this be userId?

zhx828 · 2024-07-18T02:35:08Z

src/test/java/org/mskcc/cbio/oncokb/web/rest/UsageAnalysisControllerIT.java

+      user.setFirstName(String.valueOf(userName.charAt(userName.length() - 1)));
+      user.setLastName(userName.substring(0, userName.length() - 1));


I plan to make a comment about the logic of parsing username to F/L. But I guess it doesn't matter because we don't really care how user is named?

It doesn't matter too much, but the way I have the user name setup is last name ending with the first initial. So doej break down into doe and j.

The only reason why it sort of matter is that if the data is too random then it can be challenging to debug a test so I wanted to have realistic names.

zhx828 · 2024-07-18T02:35:28Z

src/test/java/org/mskcc/cbio/oncokb/web/rest/UsageAnalysisControllerIT.java

+      return userDto;
+    }
+
+    private CompanyDTO createMockCompany(int companyIndex, String companyName) {


Suggested change

private CompanyDTO createMockCompany(int companyIndex, String companyName) {

private CompanyDTO createMockCompany(int companyId, String companyName) {

zhx828 · 2024-07-18T02:39:34Z

src/test/java/org/mskcc/cbio/oncokb/web/rest/UsageAnalysisControllerIT.java

+      return sampleUrls[Math.abs(random.nextInt() % sampleUrls.length)];
+    }
+
+    private Tuple<String, JsonObject> fetchJsonObjectFromFilesObject(


I'm a bit confused about this method. This is to fetch the last key json but backfill all keys if they do not exist in obj? Maybe add some comments here?

That is correct, but I'll add some comments.

jfkonecn · 2024-07-19T13:15:34Z

src/test/java/org/mskcc/cbio/oncokb/web/rest/UsageAnalysisControllerIT.java

+                }
+
+                // update expected response for user usage endpoint
+                if (isThisYear) {


@zhx828 I know limiting the usage metrics to only "this" year isn't what we want, but it is currently how the code works.

You expect to change this after finishing "3 years" requirement to make the test pass?

zhx828

All places where you do keys[keys.length - 1], you should expect out of bound exception. I left a comment asking whether this is your intention. If not, we need to handle it. Out of bound exception is not the best way to show why the test fail I suppose?

And thank you for wrapping logics into methods, so much easier to review!

zhx828 · 2024-07-21T21:45:16Z

src/test/java/org/mskcc/cbio/oncokb/web/rest/UsageAnalysisControllerIT.java

+     * @param value The value that is going to be added to value located in the JSON path.
+     * @param filePath First key in the files object.
+     * @param index Array index of the array the {@code filePath} parameter points to.


These should be deleted I suppose.

zhx828 · 2024-07-21T22:01:15Z

src/test/java/org/mskcc/cbio/oncokb/web/rest/UsageAnalysisControllerIT.java

+     * Get value from files JSON object based on the JSON path created by the
+     * {@code filePath}/[{@code index}]/{@code keys}.
+     * If an object is missing in the JSON path then it's created.
+     * @param filePath First key in the files object.


Is the description for filePath accurate?

Yes, I'll rename it from filePath to firstKey

zhx828 · 2024-07-21T22:05:01Z

src/test/java/org/mskcc/cbio/oncokb/web/rest/UsageAnalysisControllerIT.java

+        }
+        obj = (JsonObject) obj.get(key);
+      }
+      String lastKey = keys[keys.length - 1];


Do you intend to let the test fails by throwing out of bound exception when no key been added to this method?

It shouldn't happen since this code is just generating the mock data. I don't think people will touch this code very often, but I'll update the methods so that this exception cannot occur.

zhx828 · 2024-07-21T22:06:29Z

src/test/java/org/mskcc/cbio/oncokb/web/rest/UsageAnalysisControllerIT.java

+
+      for (int i = 0; i < keys.length - 1; i++) {
+        String key = keys[i];
+        if (!obj.has(key)) {


When key doesn't exist, should we directly return 0 instead of continuing?

Basically I'm doing the equivalent of a mkdir -p here. I want to make sure the entire path exists and also get the value from the path.

zhx828 · 2024-07-21T22:12:37Z

src/test/java/org/mskcc/cbio/oncokb/web/rest/UsageAnalysisControllerIT.java

+
+          String yearKey = String.valueOf(year);
+
+          for (int month = 1; month <= 12; month++) {


Interesting testing stratagy. I'm not testing expert. But would dynamic mock data introduces unpredictability?

Yes, but I'm hard coding the seed for the random number generator so the data is always the same unless the calls to the random number change which would only happen if we make changes to the mock data.

I thought about making just a bunch of JSON files, but I felt like this would be easier to make changes to.

That said, I could switch this to reading a bunch of JSON files instead of dynamically generating the mock data if you feel it's better to maintain.

The data is not always the same because you also use today as timestamp. The file will be different every day. Is that ok given the use case here? If it's ok, fine to me to keep the current logic.

I forgot about that part, but since the endpoint logic is dependent on today's date there isn't a way I can think of that wouldn't require changes to UsageAnalysisController. You'd have to mock out all library calls to java.time.

I don't think it's worth mocking that logic since this will only shift the data daily.

In my experience randomness is only worth addressing if it makes debugging tests harder or the tests randomly fail but failure cannot be reproduced.

I figured it out. I can use the built-in java clock object.

zhx828 · 2024-07-21T22:25:21Z

src/test/java/org/mskcc/cbio/oncokb/web/rest/UsageAnalysisControllerIT.java

+     * @param keys The keys in JSON path
+     * @return The resulting {@code KeyValuePair}
+     */
+    private KeyValuePair<String, JsonObject> fetchJsonObjectFromFilesObject(


Two fetchJsonObjectFromFilesObject methods have the same name and same return but produce completely different JSONObject, one with last index included and set the initial value and one without last index.
Can we maybe update the comment and even the method name?

I'll remove the logic to set the initial value to avoid confusion.

zhx828 · 2024-07-21T22:26:37Z

src/test/java/org/mskcc/cbio/oncokb/web/rest/UsageAnalysisControllerIT.java

+      if (!obj.has(lastKey)) {
+        obj.addProperty(lastKey, 0);
+      }


Why isn't this part of fetchJsonObjectFromFilesObject? The other fetchJsonObjectFromFilesObject method does have it.

It's not really needed in this method so I'll remove it to avoid confusion.

zhx828 · 2024-07-21T22:30:40Z

src/test/java/org/mskcc/cbio/oncokb/web/rest/UsageAnalysisControllerIT.java

+     * @param value Sets the specified value at the given JSON path.
+     * @param keys The keys in JSON path.
+     */
+    private void safeSetNestedValueInFilesObject(String value, String... keys) {


You added safe here. Do you imply a logic which could be not safe?

I called it safe because if there are missing objects then they are created for you instead of an exception being thrown.

I'll change fetchJsonObjectFromFilesObject to safeFetchJsonObjectFromFilesObject

zhx828 · 2024-07-21T22:31:52Z

src/test/java/org/mskcc/cbio/oncokb/web/rest/UsageAnalysisControllerIT.java

+      String usageResourcesEndpoint = "/api/usage/resources";
+      safeAddNestedValueInFilesObject(
+        value,
+        usageResourcesEndpoint + "?endpoint=" + oncokbEndpoint,


This can be a constant within this method.

zhx828 · 2024-07-21T22:37:38Z

src/test/java/org/mskcc/cbio/oncokb/web/rest/UsageAnalysisControllerIT.java

+  }
+
+  @Test
+  public void shouldGetUsageSummaryResources() throws Exception {


Am I correct to say these tests are to test whether the endpoints will return previously generated files properly?

Yeah they're to test what the current behavior of the endpoints are.

zhx828 · 2024-07-22T21:16:56Z

src/test/java/org/mskcc/cbio/oncokb/web/rest/UsageAnalysisControllerIT.java

+     */
+    private KeyValuePair<String, JsonObject> safeFetchJsonObjectFromFilesObject(
+      String firstKey,
+      int index,


Is it accurate to say this method is safeFetchJsonObjectFromFilesJsonArray? Also do you assume all values of restOfTheKeys are JsonObject? Is there a way we can include number as key in restOfTheKeys and get JsonArray when the key is number? A bit risky to assume that I suppose.

Kind of. It's assuming the first key points to an array of JSON objects.

Yes, it is assumed that the rest of the values are JSON objects.

It is risky to assume there will be an array, but it's only being used for this test code. The class is also scoped to just this test class so there isn't a risk someone will use it outside of this class unless they change the code. I don't it's worth making this code more generic if this is the only place it's used.

If we want absolute type safety then I should switch to classes instead of building out JSON.

zhx828 · 2024-07-22T21:17:22Z

src/test/java/org/mskcc/cbio/oncokb/web/rest/UsageAnalysisControllerIT.java

+      for (int i = 0; i < restOfTheKeys.length - 1; i++) {
+        String key = restOfTheKeys[i];
+        if (!obj.has(key)) {
+          obj.add(key, new JsonObject());
+        }
+        obj = (JsonObject) obj.get(key);
+      }
+      String lastKey = restOfTheKeys.length == 0
+        ? firstKey
+        : restOfTheKeys[restOfTheKeys.length - 1];
+      return new KeyValuePair<>(lastKey, obj);


This is the dup of 263-275

zhx828

Left few comments.

zhx828 · 2024-07-24T14:43:45Z

src/main/java/org/mskcc/cbio/oncokb/config/ClockConfig.java

+
+  @Bean
+  public Clock clock() {
+    return Clock.systemDefaultZone();


Can we make sure this is UTC, if not we should make it be.

jfkonecn force-pushed the feature/usage branch from 612923b to c716acc Compare July 9, 2024 14:17

jfkonecn requested a review from zhx828 July 9, 2024 14:26

jfkonecn marked this pull request as ready for review July 9, 2024 14:26

Added unit test to usage metrics endpoints

9ee860a

jfkonecn force-pushed the feature/usage branch from c716acc to 9ee860a Compare July 9, 2024 15:21

zhx828 requested changes Jul 18, 2024

View reviewed changes

jfkonecn added the feature Feature tag for release label Jul 18, 2024

jfkonecn added 4 commits July 18, 2024 16:50

addressed PR comments

b20d0e8

refactor

710daae

refactor

d6979d5

refactor

a2a7352

jfkonecn commented Jul 19, 2024

View reviewed changes

jfkonecn requested a review from zhx828 July 19, 2024 13:15

zhx828 requested changes Jul 21, 2024

View reviewed changes

addressed PR comments

a277dcf

jfkonecn requested a review from zhx828 July 22, 2024 15:25

zhx828 reviewed Jul 22, 2024

View reviewed changes

addressed PR comments

3ff31e7

jfkonecn requested a review from zhx828 July 23, 2024 13:47

jfkonecn added 2 commits July 23, 2024 13:15

addressed PR comments

0bd70c2

created clock bean

4d24a6e

zhx828 reviewed Jul 24, 2024

View reviewed changes

jfkonecn added 2 commits July 24, 2024 11:32

addressed PR comments

04cc5e0

addressed PR comments

d7583b2

jfkonecn requested a review from zhx828 July 24, 2024 17:00

jfkonecn merged commit 43fab0d into oncokb:master Jul 24, 2024
6 checks passed

jfkonecn deleted the feature/usage branch July 24, 2024 18:56

		user.setFirstName(String.valueOf(userName.charAt(userName.length() - 1)));
		user.setLastName(userName.substring(0, userName.length() - 1));

	private CompanyDTO createMockCompany(int companyIndex, String companyName) {
	private CompanyDTO createMockCompany(int companyId, String companyName) {


		String yearKey = String.valueOf(year);

		for (int month = 1; month <= 12; month++) {

Added unit test to usage metrics endpoints #1133

Added unit test to usage metrics endpoints #1133

Conversation

jfkonecn commented Jul 9, 2024 • edited Loading

zhx828 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

zhx828 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

jfkonecn Jul 23, 2024 • edited Loading

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

jfkonecn Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhx828 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfkonecn commented Jul 9, 2024 •

edited

Loading

jfkonecn Jul 23, 2024 •

edited

Loading

jfkonecn Jul 23, 2024 •

edited

Loading