Added version control diff #1345
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1345 +/- ##
==========================================
+ Coverage 91.48% 91.89% +0.40%
==========================================
Files 150 152 +2
Lines 10607 11032 +425
==========================================
+ Hits 9704 10138 +434
+ Misses 903 894 -9
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| @@ -426,19 +433,63 @@ def checkout(self, address: str, create: bool = False) -> str: | |||
| def log(self): | |||
| """Displays the details of all the past commits.""" | |||
| commit_node = self.version_state["commit_node"] | |||
| logger.info("---------------\nHub Version Log\n---------------\n") | |||
| logger.info(f"Current Branch: {self.version_state['branch']}") | |||
| print("---------------\nHub Version Log\n---------------\n") | |||
There was a problem hiding this comment.
Are we comprehensively changing these emits or cherry picking as we go along?
There was a problem hiding this comment.
We only have 2 functions that deliberately print out something, diff and log.
Realized that print was a better choice while implementing diff, changed log too for consistency.
There was a problem hiding this comment.
The cluster setup (atm) will capture stdout emits as log. Strongly suggest removing any free form debugging logs, and strictly stick to f"<log-level> <log-msg> - <key-0>: <val-0> - [...] <key-n>: <val-n>" formatting of logs. For debug (check with @absinha18) it is possible that stderr is ignored and can be used for free-form debugging emits. We wants logs that we can systemically grep.
| assert local_ds.commit_id != third_commit_id | ||
|
|
||
|
|
||
| def test_diff_linear(local_ds, capsys): |
There was a problem hiding this comment.
Going forward like to see a more robust approach to test parameters. We can discuss what package in python world exist that allow for parametric tests that obviate the need for hardcoded values.
There was a problem hiding this comment.
we do have parametrization with different kinds of datasets with our fixtures. This test doesn't need to use s3/gcs datasets though, so just using "local_ds" fixture works
There was a problem hiding this comment.
I was actually referring to lines 371->, so the test body itself and not the source of the data. Things like local_ds.xyz.extend([1, 2, 3]). How about something like:
def test_diff_linear(test_spec, local_ds, capsys) where test_spec is a dictionary of vars and funcs that generate values for the test body (which should be ideally entirely free of any hardcoded input). For now, you can init the test_spec with values but at a later date we can fuzz the code with randomly generated test_spec.
🚀 🚀 Pull Request
Checklist:
coverage-rateupChanges
Need to add tests.