added change to tasks file and all tests #106

femalves · 2023-07-18T16:46:14Z

No description provided.

femalves · 2023-07-18T16:49:55Z

actions/setup-python#672
"Breaking changes
Hello everyone. The Python 2.7.x will be removed from python-versions and it won't be possible to set up Python 2.7.x by setup-python.

The motivation for the changes
Python2.7 is not supported since January 1, 2020."

tjacovich · 2023-07-18T17:44:09Z

To update the tests to python3
This section in the python_actions.yaml file

  steps:
    - uses: actions/checkout@v2
    - uses: actions/setup-python@v2
      with:
        python-version: 2.7

Should become:

    steps:
      - uses: actions/checkout@v2
      - uses: actions/setup-python@v2
        with:
          python-version: "3.8"

tjacovich

Overall looks good. Just a couple of formatting things and the update to the github action.

tjacovich · 2023-07-18T17:29:14Z

ADSOrcid/tasks.py

-    cl = updater.update_record(rec, claim, app.conf.get('MIN_LEVENSHTEIN_RATIO', 0.9))
+    cl = updater.update_record(rec, claim, app.conf.get("MIN_LEVENSHTEIN_RATIO", 0.9))
+    unique_bibs = list(set([bibcode] + identifiers))
+    status = "verified" if cl else "rejected"


This works, I would just recommend rewriting it as

if ... else ...

to make it a little more readable

tjacovich · 2023-07-18T18:16:32Z

requirements.txt

-numpy==1.16.6
-matplotlib==2.0.2
+numpy==1.22 # 1.16.6 is not compatible with M1. 
+matplotlib==3.5.0 #2.0.2 is not compatible with Python 3.8


Both of these make sense, I would just delete the comments before merging.

Yup. Thanks for the reminder. Done!

tjacovich · 2023-07-18T18:29:55Z

ADSOrcid/tasks.py

+    cl = updater.update_record(rec, claim, app.conf.get("MIN_LEVENSHTEIN_RATIO", 0.9))
+    unique_bibs = list(set([bibcode] + identifiers))
+    status = "verified" if cl else "rejected"
+    r = app.client.post(


I would either move this post down to the checks on r.status_code and r.json() or move the checks up to be the next call after this post so that way all the code related to the post to ORCID service is in one spot.

tjacovich · 2023-07-19T14:45:09Z

ADSOrcid/tests/test_tasks.py

+    def test_match_claim_unknown_payload_should_return_warning(self):
+        with pytest.raises(ProcessingException) as exception_info:
+            tasks.task_match_claim([])
+        assert str(exception_info.value) == "Received unknown payload []"


in general we use the self.assertX methods provided by TestCase for checking the output of tests. There's nothing wrong with assert. It's more a consistency thing.

tjacovich · 2023-07-19T14:45:42Z

ADSOrcid/tests/test_tasks.py

+        }
+        with pytest.raises(ProcessingException) as exception_info:
+            tasks.task_match_claim(claim)
+        assert str(


same here with assert vs self.assertEqual()

tjacovich · 2023-07-19T14:49:14Z

ADSOrcid/tests/test_tasks.py

+            assert "id2" in warning_message
+            assert "BIBCODE22" in warning_message
+            assert "0000-0003-3041-2092" in warning_message
+            assert "does not match input" in warning_message


All these tests look good. I would just unify the assertions to all use TestCase.assertX

…fused

…ld_return_rejected

…return_verified

…_should_return_does_not_match

…turn_rejected back

…rified

…turn_verified

This reverts commit 62a649c.

tjacovich

This looks good to me. Just one incredibly small potential tweak in tasks.py and then one potential test tweak. It looks like one of the assert methods used in the tests for models.py has been deprecated. Would you be willing to incorporate that fix into this PR?

One of the offending lines (it looks like there might be two of them in total):

ADSOrcid/tests/test_models.py::Test::test_models
  /home/runner/work/ADSOrcid/ADSOrcid/ADSOrcid/tests/test_models.py:61: DeprecationWarning: Please use assertRaisesRegex instead.
    with self.assertRaisesRegexp(Exception, 'IntegrityError'):

tjacovich · 2023-07-26T14:59:50Z

ADSOrcid/tasks.py

-    cl = updater.update_record(rec, claim, app.conf.get('MIN_LEVENSHTEIN_RATIO', 0.9))
+    cl = updater.update_record(rec, claim, app.conf.get("MIN_LEVENSHTEIN_RATIO", 0.9))
+    unique_bibs = list(set([bibcode] + identifiers))
+    status = "verified"


This is a really small thing, but you could tuck this into the if cl: to be consistent with the else

added change to tasks file and all tests

f316776

femalves requested a review from tjacovich July 18, 2023 16:50

updated python_actions to fix build

71a9a62

tjacovich requested changes Jul 19, 2023

View reviewed changes

femalves added 3 commits July 19, 2023 11:16

fixing odd import error

4bb7ce8

making changes Taylor proposed

570bd6c

fixing tkagg import error

7b86803

femalves force-pushed the dev_status_reject_bug branch from b54ed38 to 7b86803 Compare July 20, 2023 16:43

femalves added 20 commits July 20, 2023 12:46

adding agg for testing

37b4029

creating directory

5fbdc3d

installing xvfb

270aa98

using agg just for testing

7700de6

testing with Agg

baf3e56

Testing with Agg on levenshtein_default"

950eb54

commenting out test that we assumed is holding the pipeline

5477db6

running pytest in verbose mode

6f8ced4

removing test test_logger_warning_task_match_claim_no_cl_should_be_re…

52b836a

…fused

removing test_task_match_claim_warning_no_cl_status_code_not_200_shou…

089a2b2

…ld_return_rejected

removing test_task_match_claim_warning_cl_status_code_not_200_should_…

9e764d1

…return_verified

removing test_task_match_claim_warning_cl_bibcode_length_is_different…

20e177e

…_should_return_does_not_match

running test_logger_warning_task_match_claim_no_cl_should_be_refused

359b94f

fixing warn mistake

55961d6

add test_task_match_claim_warning_no_cl_status_code_not_200_should_re…

d8f0678

…turn_rejected back

test_task_match_claim_warning_cl_status_code_not_200_should_return_ve…

a00fc78

…rified

remove test_task_match_claim_warning_cl_status_code_not_200_should_re…

843b018

…turn_verified

something weird is going on

3613783

updated problematic test

f67938b

remove matplotlibrc and update the last problematic test

1de99c3

femalves added 8 commits July 24, 2023 13:03

changing test_report back

07dfed9

changing levenshtein_default.py back

a133705

changing back to tkagg

62a649c

Revert "changing back to tkagg"

46a096e

This reverts commit 62a649c.

changing back to tkagg and adding Xvfb for testing

5cae7cb

adding line

7a68f6b

adding line

978c6fd

remove line

de9ebde

femalves requested a review from tjacovich July 24, 2023 18:24

tjacovich reviewed Jul 26, 2023

View reviewed changes

modified status position and updated deprecated assertion

f2c8059

tjacovich approved these changes Jul 28, 2023

View reviewed changes

tjacovich linked an issue Jul 28, 2023 that may be closed by this pull request

Update status to "rejected" if claim is rejected #105

Closed

femalves merged commit d2d71ac into master Jul 28, 2023
2 checks passed

tjacovich mentioned this pull request Oct 5, 2023

Bump numpy from 1.16.6 to 1.22.0 #100

Closed

femalves deleted the dev_status_reject_bug branch January 10, 2024 17:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added change to tasks file and all tests #106

added change to tasks file and all tests #106

femalves commented Jul 18, 2023

femalves commented Jul 18, 2023

tjacovich commented Jul 18, 2023 •

edited

Loading

tjacovich left a comment

tjacovich Jul 18, 2023

tjacovich Jul 18, 2023

femalves Jul 19, 2023

tjacovich Jul 18, 2023

tjacovich Jul 19, 2023

tjacovich Jul 19, 2023

tjacovich Jul 19, 2023

tjacovich left a comment

tjacovich Jul 26, 2023

added change to tasks file and all tests #106

added change to tasks file and all tests #106

Conversation

femalves commented Jul 18, 2023

femalves commented Jul 18, 2023

tjacovich commented Jul 18, 2023 • edited Loading

tjacovich 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

tjacovich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjacovich commented Jul 18, 2023 •

edited

Loading