You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/07/25 08:27:22 UTC

[GitHub] [superset] zhaoyongjie opened a new pull request, #20842: feat: a simple LRUCache in frontend

zhaoyongjie opened a new pull request, #20842:
URL: https://github.com/apache/superset/pull/20842

   ### SUMMARY
   <!--- Describe the change below, including rationale and design decisions -->
   the [Drill to detail modal](https://github.com/apache/superset/pull/20728) needs a LRUCache to handle response data in the client, so making a simple LRUCache in superset-ui/core.
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   <!--- Skip this if not applicable -->
   N/A
   
   ### TESTING INSTRUCTIONS
   <!--- Required! What steps can be taken to manually verify the changes? -->
   passes all CI
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] Has associated issue:
   - [ ] Required feature flags:
   - [ ] Changes UI
   - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
     - [ ] Migration is atomic, supports rollback & is backwards-compatible
     - [ ] Confirm DB migration upgrade and downgrade tested
     - [ ] Runtime estimates and downtime expectations provided
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] zhaoyongjie merged pull request #20842: feat: a simple LRUCache in frontend

Posted by GitBox <gi...@apache.org>.
zhaoyongjie merged PR #20842:
URL: https://github.com/apache/superset/pull/20842


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] michael-s-molina commented on a diff in pull request #20842: feat: a simple LRUCache in frontend

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #20842:
URL: https://github.com/apache/superset/pull/20842#discussion_r928957403


##########
superset-frontend/packages/superset-ui-core/test/utils/lruCache.test.ts:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { lruCache } from '@superset-ui/core';
+
+describe('simple LRUCache', () => {
+  test('initial LRU', () => {
+    expect(lruCache().capacity).toBe(100);
+    expect(lruCache(10).capacity).toBe(10);
+    expect(lruCache(10).size).toBe(0);
+    expect(() => lruCache(0)).toThrow(Error);
+  });
+
+  test('LRU', () => {

Review Comment:
   If you had inverted the process and started writing the tests first (TDD), you would probably end up listing the requirements for the cache component and writing a test for each requirement. Very similar to my suggestion.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] michael-s-molina commented on a diff in pull request #20842: feat: a simple LRUCache in frontend

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #20842:
URL: https://github.com/apache/superset/pull/20842#discussion_r928926138


##########
superset-frontend/packages/superset-ui-core/test/utils/lruCache.test.ts:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { lruCache } from '@superset-ui/core';
+
+describe('simple LRUCache', () => {
+  test('initial LRU', () => {
+    expect(lruCache().capacity).toBe(100);
+    expect(lruCache(10).capacity).toBe(10);
+    expect(lruCache(10).size).toBe(0);
+    expect(() => lruCache(0)).toThrow(Error);
+  });
+
+  test('LRU', () => {

Review Comment:
   Here's an example:
   
   ```
   const newCache = () => {
     const cache = lruCache<string>(3);
     cache.set('1', 'a');
     cache.set('2', 'b');
     cache.set('3', 'c');
     cache.set('4', 'd');
     return cache;
   };
   
   test('initial LRU', () => {
     expect(lruCache().capacity).toBe(100);
     expect(lruCache(10).capacity).toBe(10);
     expect(lruCache(10).size).toBe(0);
     expect(() => lruCache(0)).toThrow(Error);
   });
   
   test('correctly evicts LRU value', () => {
     const cache = newCache();
     expect(cache.size).toBe(3);
     expect(cache.has('1')).toBeFalsy();
     expect(cache.get('1')).toBeUndefined();
   });
   
   test('correctly updates LRU value', () => {
     const cache = newCache();
     cache.get('2');
     cache.set('5', 'e');
     expect(cache.has('2')).toBeTruthy();
     expect(cache.has('3')).toBeFalsy();
   });
   
   test('throws exception when key is invalid', () => {
     const cache = lruCache<string>(3);
     // @ts-ignore
     expect(() => cache.set(0)).toThrow(TypeError);
   });
   
   test('clears the cache', () => {
     const cache = newCache();
     cache.clear();
     expect(cache.size).toBe(0);
     expect(cache.capacity).toBe(3);
   });
   ```
   
   This way, when a test fails, it's easier to identify what parts of the code are failing. Also, you don't have dependencies between different test cases. You can also optimize the initial state for each test if desired.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] zhaoyongjie commented on pull request #20842: feat: a simple LRUCache in frontend

Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on PR #20842:
URL: https://github.com/apache/superset/pull/20842#issuecomment-1195087016

   @michael-s-molina @ktmud @stephenLYZ Thanks for the review, I will put your consideration into future iterations, if more requirements arise, use the open source solution.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] codecov[bot] commented on pull request #20842: feat: a simple LRUCache in frontend

Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #20842:
URL: https://github.com/apache/superset/pull/20842#issuecomment-1193803512

   # [Codecov](https://codecov.io/gh/apache/superset/pull/20842?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#20842](https://codecov.io/gh/apache/superset/pull/20842?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (66a2019) into [master](https://codecov.io/gh/apache/superset/commit/9bf7ed58cdc1d5523d0cb661f8fdbf7df9b10fe7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9bf7ed5) will **increase** coverage by `0.01%`.
   > The diff coverage is `100.00%`.
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #20842      +/-   ##
   ==========================================
   + Coverage   66.33%   66.34%   +0.01%     
   ==========================================
     Files        1756     1757       +1     
     Lines       66769    66789      +20     
     Branches     7059     7063       +4     
   ==========================================
   + Hits        44288    44308      +20     
     Misses      20684    20684              
     Partials     1797     1797              
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | javascript | `52.00% <100.00%> (+0.02%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/20842?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...nd/packages/superset-ui-core/src/utils/lruCache.ts](https://codecov.io/gh/apache/superset/pull/20842/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvdXRpbHMvbHJ1Q2FjaGUudHM=) | `100.00% <100.00%> (ø)` | |
   
   Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] ktmud commented on a diff in pull request #20842: feat: a simple LRUCache in frontend

Posted by GitBox <gi...@apache.org>.
ktmud commented on code in PR #20842:
URL: https://github.com/apache/superset/pull/20842#discussion_r929069059


##########
superset-frontend/packages/superset-ui-core/test/utils/lruCache.test.ts:
##########
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { lruCache } from '@superset-ui/core';
+
+test('initial LRU', () => {
+  expect(lruCache().capacity).toBe(100);
+  expect(lruCache(10).capacity).toBe(10);
+  expect(lruCache(10).size).toBe(0);
+  expect(() => lruCache(0)).toThrow(Error);
+});
+
+test('LRU operation', () => {
+  const cache = lruCache<string>(3);
+  cache.set('1', 'a');
+  cache.set('2', 'b');
+  cache.set('3', 'c');
+  cache.set('4', 'd');
+  expect(cache.size).toBe(3);
+  expect(cache.has('1')).toBeFalsy();
+  expect(cache.get('1')).toBeUndefined();
+  cache.get('2');
+  cache.set('5', 'e');
+  expect(cache.has('2')).toBeTruthy();
+  expect(cache.has('3')).toBeFalsy();
+  // @ts-ignore
+  expect(() => cache.set(0)).toThrow(TypeError);

Review Comment:
   ```suggestion
     // @ts-expect-error
     expect(() => cache.set('set-key-with-no-value')).toThrow(TypeError);
   ```
   
   I think `ts-expect-error` is now preferred for such use cases. Also, we can make a small change to indicate why an error is expected. 
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] michael-s-molina commented on a diff in pull request #20842: feat: a simple LRUCache in frontend

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #20842:
URL: https://github.com/apache/superset/pull/20842#discussion_r928932598


##########
superset-frontend/packages/superset-ui-core/test/utils/lruCache.test.ts:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { lruCache } from '@superset-ui/core';
+
+describe('simple LRUCache', () => {
+  test('initial LRU', () => {
+    expect(lruCache().capacity).toBe(100);
+    expect(lruCache(10).capacity).toBe(10);
+    expect(lruCache(10).size).toBe(0);
+    expect(() => lruCache(0)).toThrow(Error);
+  });
+
+  test('LRU', () => {

Review Comment:
   Here's an example:
   
   ```
   const newCache = () => {
     const cache = lruCache<string>(3);
     cache.set('1', 'a');
     cache.set('2', 'b');
     cache.set('3', 'c');
     return cache;
   };
   
   test('initial LRU', () => {
     expect(lruCache().capacity).toBe(100);
     expect(lruCache(10).capacity).toBe(10);
     expect(lruCache(10).size).toBe(0);
     expect(() => lruCache(0)).toThrow(Error);
   });
   
   test('correctly evicts LRU value', () => {
     const cache = newCache();
     expect(cache.size).toBe(3);
     cache.set('4', 'd');
     expect(cache.has('1')).toBeFalsy();
     expect(cache.get('1')).toBeUndefined();
   });
   
   test('correctly updates LRU value', () => {
     const cache = newCache();
     cache.get('1');
     cache.set('5', 'e');
     expect(cache.has('1')).toBeTruthy();
     expect(cache.has('2')).toBeFalsy();
   });
   
   test('throws exception when key is invalid', () => {
     const cache = lruCache<string>(3);
     // @ts-ignore
     expect(() => cache.set(0)).toThrow(TypeError);
   });
   
   test('clears the cache', () => {
     const cache = newCache();
     cache.clear();
     expect(cache.size).toBe(0);
     expect(cache.capacity).toBe(3);
   });
   ```
   
   This way, when a test fails, it's easier to identify what parts of the code are broken. It also avoids state dependencies between different test cases. You can reuse an initial state or optimize for each test.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] michael-s-molina commented on a diff in pull request #20842: feat: a simple LRUCache in frontend

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #20842:
URL: https://github.com/apache/superset/pull/20842#discussion_r928952951


##########
superset-frontend/packages/superset-ui-core/test/utils/lruCache.test.ts:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { lruCache } from '@superset-ui/core';
+
+describe('simple LRUCache', () => {
+  test('initial LRU', () => {
+    expect(lruCache().capacity).toBe(100);
+    expect(lruCache(10).capacity).toBe(10);
+    expect(lruCache(10).size).toBe(0);
+    expect(() => lruCache(0)).toThrow(Error);
+  });
+
+  test('LRU', () => {

Review Comment:
   I think readability and separation of concerns are more important than the number of lines. It's not a single test case because they are different requirements. You could have a bug related to value eviction but have the requirements for clearing the cache and type checking the keys still working.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] michael-s-molina commented on a diff in pull request #20842: feat: a simple LRUCache in frontend

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #20842:
URL: https://github.com/apache/superset/pull/20842#discussion_r929021504


##########
superset-frontend/packages/superset-ui-core/test/utils/lruCache.test.ts:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { lruCache } from '@superset-ui/core';
+
+describe('simple LRUCache', () => {
+  test('initial LRU', () => {
+    expect(lruCache().capacity).toBe(100);
+    expect(lruCache(10).capacity).toBe(10);
+    expect(lruCache(10).size).toBe(0);
+    expect(() => lruCache(0)).toThrow(Error);
+  });
+
+  test('LRU', () => {

Review Comment:
   I see no problem in merging these two tests together since they are highly related 😉 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20842: feat: a simple LRUCache in frontend

Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20842:
URL: https://github.com/apache/superset/pull/20842#discussion_r928947089


##########
superset-frontend/packages/superset-ui-core/test/utils/lruCache.test.ts:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { lruCache } from '@superset-ui/core';
+
+describe('simple LRUCache', () => {
+  test('initial LRU', () => {
+    expect(lruCache().capacity).toBe(100);
+    expect(lruCache(10).capacity).toBe(10);
+    expect(lruCache(10).size).toBe(0);
+    expect(() => lruCache(0)).toThrow(Error);
+  });
+
+  test('LRU', () => {

Review Comment:
   This is just a test case, do we really need "modularity"? the original test case has 19 lines and the suggestion from you has 32 lines(removed empty line between test case). 
   
   ![image](https://user-images.githubusercontent.com/2016594/180799537-75863a2e-0ba6-4911-a54c-bb7d4af15058.png)
   
   the `newCache` is also not a setup fixture, we can't use it in `initial LRU` and `LRU handle null and undefined`. This is a completely additional abstraction.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] michael-s-molina commented on a diff in pull request #20842: feat: a simple LRUCache in frontend

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #20842:
URL: https://github.com/apache/superset/pull/20842#discussion_r928990861


##########
superset-frontend/packages/superset-ui-core/test/utils/lruCache.test.ts:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { lruCache } from '@superset-ui/core';
+
+describe('simple LRUCache', () => {
+  test('initial LRU', () => {
+    expect(lruCache().capacity).toBe(100);
+    expect(lruCache(10).capacity).toBe(10);
+    expect(lruCache(10).size).toBe(0);
+    expect(() => lruCache(0)).toThrow(Error);
+  });
+
+  test('LRU', () => {

Review Comment:
   The code that I sent you is just an example. Feel free to improve it. For me, the important part is that the each requirement is in its own test case.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20842: feat: a simple LRUCache in frontend

Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20842:
URL: https://github.com/apache/superset/pull/20842#discussion_r928900108


##########
superset-frontend/packages/superset-ui-core/test/utils/lruCache.test.ts:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { lruCache } from '@superset-ui/core';
+
+describe('simple LRUCache', () => {

Review Comment:
   done



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] michael-s-molina commented on a diff in pull request #20842: feat: a simple LRUCache in frontend

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #20842:
URL: https://github.com/apache/superset/pull/20842#discussion_r928980157


##########
superset-frontend/packages/superset-ui-core/test/utils/lruCache.test.ts:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { lruCache } from '@superset-ui/core';
+
+describe('simple LRUCache', () => {
+  test('initial LRU', () => {
+    expect(lruCache().capacity).toBe(100);
+    expect(lruCache(10).capacity).toBe(10);
+    expect(lruCache(10).size).toBe(0);
+    expect(() => lruCache(0)).toThrow(Error);
+  });
+
+  test('LRU', () => {

Review Comment:
   If you move the expect after the set then they will be.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20842: feat: a simple LRUCache in frontend

Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20842:
URL: https://github.com/apache/superset/pull/20842#discussion_r929012269


##########
superset-frontend/packages/superset-ui-core/test/utils/lruCache.test.ts:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { lruCache } from '@superset-ui/core';
+
+describe('simple LRUCache', () => {
+  test('initial LRU', () => {
+    expect(lruCache().capacity).toBe(100);
+    expect(lruCache(10).capacity).toBe(10);
+    expect(lruCache(10).size).toBe(0);
+    expect(() => lruCache(0)).toThrow(Error);
+  });
+
+  test('LRU', () => {

Review Comment:
   @michael-s-molina Sorry, my point was that each call to `get` and `set` has an effect on the entire LRU, for example, the `correctly evicts LRU value` and the `correctly updates LRU value` should operate on a same LRU instance rather than separately. For this argument, we should probably get someone else to take a look at it as well, and I'd be happy to revise it, thank you very much for the review.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20842: feat: a simple LRUCache in frontend

Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20842:
URL: https://github.com/apache/superset/pull/20842#discussion_r928947089


##########
superset-frontend/packages/superset-ui-core/test/utils/lruCache.test.ts:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { lruCache } from '@superset-ui/core';
+
+describe('simple LRUCache', () => {
+  test('initial LRU', () => {
+    expect(lruCache().capacity).toBe(100);
+    expect(lruCache(10).capacity).toBe(10);
+    expect(lruCache(10).size).toBe(0);
+    expect(() => lruCache(0)).toThrow(Error);
+  });
+
+  test('LRU', () => {

Review Comment:
   This is just a test case, do we really need "modularity"? the original test case has 19 lines and the suggestion from you has 32 lines(removed empty line between test case). 
   
   ![image](https://user-images.githubusercontent.com/2016594/180799537-75863a2e-0ba6-4911-a54c-bb7d4af15058.png)
   
   the `newCache` is also not a setup fixture, we can't use it in `initial LRU` and `LRU handle null and undefined`. This is a completely additional abstraction,



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20842: feat: a simple LRUCache in frontend

Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20842:
URL: https://github.com/apache/superset/pull/20842#discussion_r929533513


##########
superset-frontend/packages/superset-ui-core/test/utils/lruCache.test.ts:
##########
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { lruCache } from '@superset-ui/core';
+
+test('initial LRU', () => {
+  expect(lruCache().capacity).toBe(100);
+  expect(lruCache(10).capacity).toBe(10);
+  expect(lruCache(10).size).toBe(0);
+  expect(() => lruCache(0)).toThrow(Error);
+});
+
+test('LRU operation', () => {
+  const cache = lruCache<string>(3);
+  cache.set('1', 'a');
+  cache.set('2', 'b');
+  cache.set('3', 'c');
+  cache.set('4', 'd');
+  expect(cache.size).toBe(3);
+  expect(cache.has('1')).toBeFalsy();
+  expect(cache.get('1')).toBeUndefined();
+  cache.get('2');
+  cache.set('5', 'e');
+  expect(cache.has('2')).toBeTruthy();
+  expect(cache.has('3')).toBeFalsy();
+  // @ts-ignore
+  expect(() => cache.set(0)).toThrow(TypeError);

Review Comment:
   currently, LRU supports to set a key without value, the default fallback value is `undefined`. I totally agree with using node-lru-cache if we have more requirements arise. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] michael-s-molina commented on a diff in pull request #20842: feat: a simple LRUCache in frontend

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #20842:
URL: https://github.com/apache/superset/pull/20842#discussion_r928775729


##########
superset-frontend/packages/superset-ui-core/test/utils/lruCache.test.ts:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { lruCache } from '@superset-ui/core';
+
+describe('simple LRUCache', () => {

Review Comment:
   [Avoid nesting when you're testing](https://github.com/apache/superset/wiki/Testing-Guidelines-and-Best-Practices#avoid-nesting-when-youre-testing)



##########
superset-frontend/packages/superset-ui-core/test/utils/lruCache.test.ts:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { lruCache } from '@superset-ui/core';
+
+describe('simple LRUCache', () => {
+  test('initial LRU', () => {
+    expect(lruCache().capacity).toBe(100);
+    expect(lruCache(10).capacity).toBe(10);
+    expect(lruCache(10).size).toBe(0);
+    expect(() => lruCache(0)).toThrow(Error);
+  });
+
+  test('LRU', () => {

Review Comment:
   Can we break this into multiple tests? We could have one to test eviction, one to test invalid key types, and one to test cache clearing.



##########
superset-frontend/packages/superset-ui-core/src/utils/lruCache.ts:
##########
@@ -0,0 +1,70 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+class LRUCache<T> {
+  private cache: Map<string, T>;
+
+  readonly capacity: number;
+
+  constructor(capacity: number) {
+    if (capacity < 1) {
+      throw new Error('The capacity in LRU must be greater than 0.');
+    }
+    this.capacity = capacity;
+    this.cache = new Map<string, T>();
+  }
+
+  public has(key: string): boolean {
+    return this.cache.has(key);
+  }
+
+  public get(key: string): T | undefined {
+    if (this.cache.has(key)) {
+      const tmp = this.cache.get(key) as T;
+      this.cache.delete(key);
+      this.cache.set(key, tmp);
+      return tmp;
+    }
+

Review Comment:
   ```suggestion
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] michael-s-molina commented on a diff in pull request #20842: feat: a simple LRUCache in frontend

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #20842:
URL: https://github.com/apache/superset/pull/20842#discussion_r928952951


##########
superset-frontend/packages/superset-ui-core/test/utils/lruCache.test.ts:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { lruCache } from '@superset-ui/core';
+
+describe('simple LRUCache', () => {
+  test('initial LRU', () => {
+    expect(lruCache().capacity).toBe(100);
+    expect(lruCache(10).capacity).toBe(10);
+    expect(lruCache(10).size).toBe(0);
+    expect(() => lruCache(0)).toThrow(Error);
+  });
+
+  test('LRU', () => {

Review Comment:
   I think readability and separation of concerns are more important than the number of lines. It's not a single test case because they are different requirements. You could have a bug related to a value eviction but have the requirements for clearing the cache and type checking the keys still working.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20842: feat: a simple LRUCache in frontend

Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20842:
URL: https://github.com/apache/superset/pull/20842#discussion_r928904842


##########
superset-frontend/packages/superset-ui-core/test/utils/lruCache.test.ts:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { lruCache } from '@superset-ui/core';
+
+describe('simple LRUCache', () => {
+  test('initial LRU', () => {
+    expect(lruCache().capacity).toBe(100);
+    expect(lruCache(10).capacity).toBe(10);
+    expect(lruCache(10).size).toBe(0);
+    expect(() => lruCache(0)).toThrow(Error);
+  });
+
+  test('LRU', () => {

Review Comment:
   Thanks for the review. This test case is for entire LRU operations.  the L43 depends on the cache instance. The purpose of this test case is to prevent incorrect keys from being inserted at runtime, rather than compile time. the `clear()` method also depends on the cache instance.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20842: feat: a simple LRUCache in frontend

Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20842:
URL: https://github.com/apache/superset/pull/20842#discussion_r928904842


##########
superset-frontend/packages/superset-ui-core/test/utils/lruCache.test.ts:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { lruCache } from '@superset-ui/core';
+
+describe('simple LRUCache', () => {
+  test('initial LRU', () => {
+    expect(lruCache().capacity).toBe(100);
+    expect(lruCache(10).capacity).toBe(10);
+    expect(lruCache(10).size).toBe(0);
+    expect(() => lruCache(0)).toThrow(Error);
+  });
+
+  test('LRU', () => {

Review Comment:
   Thanks for the review. This test case is for entire LRU operations.  the L43 depends on the cache instance. The purpose of this test case is to prevent incorrect keys from being inserted at runtime, rather than compile time. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] michael-s-molina commented on a diff in pull request #20842: feat: a simple LRUCache in frontend

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #20842:
URL: https://github.com/apache/superset/pull/20842#discussion_r928957403


##########
superset-frontend/packages/superset-ui-core/test/utils/lruCache.test.ts:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { lruCache } from '@superset-ui/core';
+
+describe('simple LRUCache', () => {
+  test('initial LRU', () => {
+    expect(lruCache().capacity).toBe(100);
+    expect(lruCache(10).capacity).toBe(10);
+    expect(lruCache(10).size).toBe(0);
+    expect(() => lruCache(0)).toThrow(Error);
+  });
+
+  test('LRU', () => {

Review Comment:
   If you had inverted the process and started writing the tests first (TDD), you'll probably end up listing the requirements for the cache component and writing a test for each requirement. Very similar to my suggestion.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20842: feat: a simple LRUCache in frontend

Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20842:
URL: https://github.com/apache/superset/pull/20842#discussion_r929012269


##########
superset-frontend/packages/superset-ui-core/test/utils/lruCache.test.ts:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { lruCache } from '@superset-ui/core';
+
+describe('simple LRUCache', () => {
+  test('initial LRU', () => {
+    expect(lruCache().capacity).toBe(100);
+    expect(lruCache(10).capacity).toBe(10);
+    expect(lruCache(10).size).toBe(0);
+    expect(() => lruCache(0)).toThrow(Error);
+  });
+
+  test('LRU', () => {

Review Comment:
   @michael-s-molina Sorry, my point was that each call to `get` and `set` has an effect on the entire LRU, for example, the `correctly evicts LRU value` and the `correctly updates LRU value` should operate on a single LRU instance rather than separately. For this argument, we should probably get someone else to take a look at it as well, and I'd be happy to revise it, thank you very much for the review.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20842: feat: a simple LRUCache in frontend

Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20842:
URL: https://github.com/apache/superset/pull/20842#discussion_r928976830


##########
superset-frontend/packages/superset-ui-core/test/utils/lruCache.test.ts:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { lruCache } from '@superset-ui/core';
+
+describe('simple LRUCache', () => {
+  test('initial LRU', () => {
+    expect(lruCache().capacity).toBe(100);
+    expect(lruCache(10).capacity).toBe(10);
+    expect(lruCache(10).size).toBe(0);
+    expect(() => lruCache(0)).toThrow(Error);
+  });
+
+  test('LRU', () => {

Review Comment:
   a modularity test case and all-in-one test case are not equivalent, because the each `get` or `set` method will affect the entire LRU.
   For example: 
   ```javascript
   test('LRU operation', () => {
     const cache = lruCache<string>(3);
     cache.set('1', 'a');
     cache.set('2', 'b');
     cache.set('3', 'c');
     cache.set('4', 'd');
     expect(cache.size).toBe(3);
   ....
   ```
   
   not equal to
   
   ```javascript
   const newCache = () => {
     const cache = lruCache<string>(3);
     cache.set('1', 'a');
     cache.set('2', 'b');
     cache.set('3', 'c');
     return cache;
   };
   test('correctly evicts LRU value', () => {
     const cache = newCache();
     expect(cache.size).toBe(3);
     cache.set('4', 'd');
   ....
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org