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

Fix Oracle SSL tests #15260

Merged
merged 2 commits into from Mar 31, 2021
Merged

Fix Oracle SSL tests #15260

merged 2 commits into from Mar 31, 2021

Conversation

jeff303
Copy link
Contributor

@jeff303 jeff303 commented Mar 19, 2021

Define new test for Oracle SSL connectivity, in oracle_test.clj, similar to how things work in mysql_test.clj

Add new test util macro, with-env-keys-renamed-by, to support running tests with environ keys temporarily renamed

Using new test macro from both MySQL and Oracle SSL connectivity tests

Removing now unneeded be-tests-oracle-ssl-ee CircleCI job

Removing now unneeded test-selector parameter for test-driver orbv in CircleCI config.yml

@jeff303
Copy link
Contributor Author

jeff303 commented Mar 19, 2021

Builds on top of #15172.

@codecov
Copy link

codecov bot commented Mar 19, 2021

Codecov Report

Merging #15260 (b5995d0) into master (54f6a5e) will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #15260      +/-   ##
==========================================
- Coverage   84.39%   84.37%   -0.02%     
==========================================
  Files         398      398              
  Lines       31056    31056              
  Branches     2240     2239       -1     
==========================================
- Hits        26209    26204       -5     
- Misses       2607     2613       +6     
+ Partials     2240     2239       -1     
Impacted Files Coverage Δ
src/metabase/driver/sql_jdbc/connection.clj 75.67% <0.00%> (-6.76%) ⬇️
src/metabase/models/query.clj 85.41% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 54f6a5e...b5995d0. Read the comment docs.

@dpsutton
Copy link
Contributor

dpsutton commented Mar 23, 2021

couple things about your macro with-env-keys-renamed-by
It iterates through every key. I think you could probably just leverage clojure.set/rename-keys

(tu/with-env-keys-whatever {:mb-mysql-test :mb-mysql-ssl-test} 
    (is (= ....))

Or you could take an arbitrary function f and call it on the env and redef with that value.

(tu/with-env-keys-whatever #(set/rename-keys % {:mb-mysql-test :mb-mysql-ssl-test})
  (is (= ...)))

That would allow more manipulation (if desired) than just renaming keys. You could duplicate, swap, what have you. But the api to take in every key as a string and return a new string seems a bit off.

Also, perhaps assert that the new env is different than the incoming environment? I imagine if you get this wrong and it doesn't update anything and your test fails for some weird reason it might be confusing. Or perhaps even worse, you get the key name wrong and your test still passes.

EDIT:
I just realized that you are swapping the prefix out, which is why you did that. Maybe that is the way to go. I think there might be some benefit to the rename keys where its more clear which keys are getting renamed but way less sure now. Feel free to ignore if none of this swayed you.

@jeff303
Copy link
Contributor Author

jeff303 commented Mar 23, 2021

couple things about your macro with-env-keys-renamed-by
It iterates through every key. I think you could probably just leverage clojure.set/rename-keys

(tu/with-env-keys-whatever {:mb-mysql-test :mb-mysql-ssl-test} 
    (is (= ....))

Or you could take an arbitrary function f and call it on the env and redef with that value.

(tu/with-env-keys-whatever #(set/rename-keys % {:mb-mysql-test :mb-mysql-ssl-test})
  (is (= ...)))

That would allow more manipulation (if desired) than just renaming keys. You could duplicate, swap, what have you. But the api to take in every key as a string and return a new string seems a bit off.

Also, perhaps assert that the new env is different than the incoming environment? I imagine if you get this wrong and it doesn't update anything and your test fails for some weird reason it might be confusing. Or perhaps even worse, you get the key name wrong and your test still passes.

EDIT:
I just realized that you are swapping the prefix out, which is why you did that. Maybe that is the way to go. I think there might be some benefit to the rename keys where its more clear which keys are getting renamed but way less sure now. Feel free to ignore if none of this swayed you.

Yeah, the intent was to have a convention that a certain prefix is renamed, in bulk, to cover things like variants of the same env (like SSL, or not, possibly other auth mechanisms or tweaks in the future). That's why I wanted to avoid having to individually enumerate the keys. However, the "by function" semantic is definitely compelling, as it is more generic (which is on brand for a test util macro). Will have to give it a bit more thought.

Copy link
Member

@camsaul camsaul left a comment

Choose a reason for hiding this comment

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

  1. You should alias namespaces you require and use those aliases, so it's clear at a glance that you're not using things without requiring them and causing namespace loading race conditions
  2. New test util functions should be "imported" by Potemkin in metabase.test, and people using them should use them via mt/. Don't use tu/ or other stuff in new tests -- we don't want to be back in the place where people had to use 6 different test util namespaces to get anything done
  3. Your macro should include some testing context to make debugging failing tests a little easier. Maybe you accidentally renamed all the env vars; you shouldn't need to debug the test util to figure that out -- it should tell you

@jeff303 jeff303 requested a review from camsaul March 24, 2021 19:23
Base automatically changed from support-mysql-pem-certificate to master March 24, 2021 19:44
Define new test for Oracle SSL connectivity, in oracle_test.clj, similar to how things work in mysql_test.clj

Add new test util macro, with-env-keys-renamed-by, to support running tests with environ keys temporarily renamed

Using new test macro from both MySQL and Oracle SSL connectivity tests

Removing now unneeded be-tests-oracle-ssl-ee CircleCI job

Removing now unneeded test-selector parameter for test-driver orbv in CircleCI config.yml

Updating JVM_OPTS to use a trust store that starts with cacerts and adds the RDS root CA, rather than one only
containing the RDS root CA
Move bulk of logic from with-env-keys-renamed-by to new do-with-env-keys-renamed-by fn instead, calling that from macro

Change logic of do-with-env-keys-renamed-by to first build the map of renames, then call set/rename-keys with that (for easier capturing of the testing context information)

Switch to u/format-color in tests

Reference new test macro via metabase.test (which imports it)

Use namespace prefixes for the delegated test calls
Copy link
Member

@camsaul camsaul left a comment

Choose a reason for hiding this comment

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

Cool LGTM

@jeff303 jeff303 merged commit c938fb0 into master Mar 31, 2021
@jeff303 jeff303 deleted the fix-oracle-ssl-tests branch March 31, 2021 15:06
@jeff303 jeff303 added this to the 0.39 milestone Mar 31, 2021
@rlotun rlotun removed this from the 0.39 milestone Apr 9, 2021
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.

None yet

4 participants