Fix tests/methods that refer to private functions in Python #7450

kevinlee12 · 2019-08-24T05:37:50Z

There are currently a few tests that reference private functions in Python. Tests should test through public interface. If the test tests a private method, find a way to test the method through the existing public interface instead (you'll need to find the corresponding public API function that uses the internal private one, and use that function instead in tests). Please note that modifications to other classes outside of the file in question may be necessary.

Note that you can identify such references by searching for the string # pylint: disable=protected-access. (If you don't see the string in the file(s) you're assigned to, please let the author of this issue know so that the list can be edited accordingly.)

Important: In general, do not just change methods from private to public. You'll need to find the already existing publicly-accessible function which invokes the private method and use that one instead.

The following the cases where we test private functions:

gae_models.VersionedModel @Azrecycle4
- delete
- revert
- get_version
- get_multi_versions
- get_snapshots_metadata
suggestion_services_test.SuggestionServicesUnitTests @gopivaibhav
- test_accept_suggestion_commit_message_after_updating_a_suggestion
- test_update_translation_suggestion_to_change_translation_html
suggestion_services_test.SuggestionIntegrationTests @hardikkat24
- setUp
email_manager_test.EmailRightsTest @vijaykpatel
- test_sender_id_validation
email_manager.BulkEmailsTests @brianlinUM
- test_that_correct_email_is_sent
- test_that_exception_is_raised_for_unauthorised_sender
library_test.LibraryPageTests @brianlinUM
- test_library_handler_for_created_explorations
build_test (All) @Mohitbalwani26
suggestion_test.SuggestionUnitTests @AnuragVats007
- setUp
exp_services_test.ZipFileExportUnitTests (all) @ayushjaink8

Completed:

The text was updated successfully, but these errors were encountered:

ykabusalah · 2019-08-30T18:23:21Z

Can I try to do this?

kevinlee12 · 2019-08-31T02:04:28Z

Sure, which part would you like to do? (each major bullet point is open)

lilithxxx · 2019-09-02T14:14:17Z

@kevinlee12 perhaps swap the bullet points with check-boxes?

kevinlee12 · 2019-09-02T17:44:58Z

good call, done!

xoxwaw · 2019-09-25T20:17:27Z

Hi can I take jobs_test.JobManagerUnitTests ?

kevinlee12 · 2019-09-26T02:33:23Z

sure, go for it!

xoxwaw · 2019-09-27T04:54:50Z

@ankita240796 could you please add @kevinlee12 as my reviewer? Thank you

kevinlee12 · 2019-09-27T05:01:45Z

I'll go ahead and do a review :)

KristiKovacs · 2019-10-03T14:13:24Z

Hello! I'd like to try: email_manager.BulkEmailsTests

DubeySandeep · 2019-10-03T14:21:47Z

Hi @KristiKovacs, I've assigned you to email_manager.BulkEmailsTests. You can start working on it! :)

rahmot · 2019-10-04T10:05:47Z

Hello @DubeySandeep, I'd like to try "suggestion_services_test.SuggestionServicesUnitTests".

DubeySandeep · 2019-10-04T10:07:40Z

@rahmot, I've assigned you to suggestion_services_test.SuggestionServicesUnitTests.

rahmot · 2019-10-04T15:04:11Z

@DubeySandeep, please can you put me through on how to get started with suggestion_services_test.SuggestionServicesUnitTests. Thank you!

DubeySandeep · 2019-10-04T15:29:55Z

@rahmot I would have approached this issue in the following way:

After going through the details of the issue, I understand that "Few test methods in test files are using private methods for testing, so I have to change the test methods such that it only uses the public methods."

Let's solve one test method for your case:

I looked into suggestion_services_test and found test_accept_suggestion_handled_suggestion_failure method on line 350.
I see line 368 is calling a private method _update_suggestion.
I read the test code and found that we are trying to change the status of the suggestion and update the suggestion using the private method from the suggestion_service.
Going further I read a few lines of suggestion_service and found mark_review_completed which does the same thing and I think that can be useful.

With all this information I will go ahead and change the test method and create a PR!

(Note: maybe this idea won't work, then you have to take a deeper look and find another way to solve the problem.)

I hope this helps.

rahmot · 2019-10-04T16:13:34Z

@DubeySandeep, thanks for the illumination. I sincerely appreciate.

Hudda · 2019-10-05T13:02:29Z

Hello @DubeySandeep can I take email_manager.DuplicateEmailTests ?

DubeySandeep · 2019-10-05T13:06:04Z

Hi @Hudda, I think you haven't filled the survey form yet, would you mind doing that first? Once you complete the instructions provided here, do let us know we will go ahead and assign you to the issue you have selcted. :)

Hudda · 2019-10-05T13:38:55Z

@DubeySandeep I have adhered to your instructions and filled the form.

DubeySandeep · 2019-10-05T15:54:05Z

@Hudda, I've assigned you email_manager.DuplicateEmailTests, you can go ahead and start working on it! :)

vojtechjelinek · 2022-02-14T18:58:22Z

@AnuragVats007 Done.

gopivaibhav · 2022-02-15T06:28:34Z

Hey @vojtechjelinek Can you please assign me suggestion_services_test.SuggestionServicesUnitTests ?
Thank you

vojtechjelinek · 2022-02-15T13:22:30Z

@gopivaibhav Done.

Mohitbalwani26 · 2022-02-15T18:22:36Z

Hey @vojtechjelinek, I was looking into file scripts/build_test.py as it is mentioned in the issue. It contains tests for functions like _minify, _minify_and_create_sourcemap, etc. which are defined in scripts/build.py, shouldn't these be made public in build.py in the first place like other functions get_dependencies_filepaths? If you could help me understand why they are private, it would be helpful.

vojtechjelinek · 2022-02-15T20:30:04Z

@Mohitbalwani26 They are private because they are not used outside of the build.py file.

Mohitbalwani26 · 2022-02-24T18:28:49Z

@Mohitbalwani26 They are private because they are not used outside of the build.py file.

@vojtechjelinek I was thinking to test these functions indirectly.
For example -

oppia/scripts/build_test.py

Line 72 in 33ff571

    
           build._minify(INVALID_INPUT_FILEPATH, INVALID_OUTPUT_FILEPATH)  # pylint: disable=protected-access

this _minify() can also be tested if we replace _minify() with minify_func() defined in build.py as it internally calls _minify according to some conditions and we can explicitly make sure that it meets the criteria to go through.

similar is case with _ensure_files_exist()

oppia/scripts/build_test.py

Line 100 in 33ff571

    
           build._ensure_files_exist(non_existent_filepaths)  # pylint: disable=protected-access

this test can be merged with test of safe_delete_file() as it internally checks the existence of file by calling _ensure_files_exist().

Does this approach look good to you?

vojtechjelinek · 2022-02-24T23:42:18Z

@Mohitbalwani26 Sounds good!

ayushjaink8 · 2022-02-25T16:11:59Z

@vojtechjelinek Can you please assign me this file: exp_services_test.ZipFileExportUnitTests (all)

vojtechjelinek · 2022-02-25T20:53:55Z

@ayushjaink8 Sure.

Mohitbalwani26 · 2022-02-27T10:27:39Z

@vojtechjelinek can you assign me build_test.py? Also i have raised a PR for the same.

vojtechjelinek · 2022-02-27T14:23:07Z

@Mohitbalwani26 Done.

jordyparker · 2022-02-27T20:36:28Z

@vojtechjelinek is there any other file i can work on ? if yes, pls assign me to it

…15020) * fix testing of private method: _minify and _ensure_files_exist * changes as suggested in CR * change filename variable value * change filename variable * fix lint issue

…Tests (#15001) Co-authored-by: Gopi Vaibhav <gopivaibhav@pop-os.localdomain>

JeeveshGarg · 2022-07-15T17:22:36Z

@vojtechjelinek Please update the issue description, unassign contributors from tasks they are no longer working on, and remove completed tasks. Add additional task if any arise.

Cross check whether the GSoC project clashes with this issue. If so, let @JeeveshGarg know. in order to prevent problems for new contributors. If not, simply respond to this.

Thanks.

vojtechjelinek · 2022-07-15T21:14:51Z

@JeeveshGarg Hey, can someone from the dev experience or automated QA overtake this? I no longer focus on this area.

JeeveshGarg · 2022-07-16T02:08:08Z

@oppia/automated-qa-reviewers @U8NWXD @krishnarao22 ?

U8NWXD · 2022-07-24T06:06:10Z

@JeeveshGarg I still need to do an audit to update this issue based on the current state of the code base. It'll have to wait until other dev workflow issues like the flakes are under control. I'd recommend removing this as a starter issue for now

kevinlee12 added backend talk-to: @kevinlee12 good first issue labels Aug 24, 2019

kevinlee12 changed the title ~~Fix tests that refer to private functions in Python~~ Fix tests/methods that refer to private functions in Python Aug 24, 2019

DubeySandeep pinned this issue Aug 24, 2019

xoxwaw mentioned this issue Sep 27, 2019

Fix part of #7450: Modify private methods in JobManagerUnitTests #7703

Closed

9 tasks

DubeySandeep added the Hacktoberfest label Sep 30, 2019

This was referenced Oct 6, 2019

Fix part of #7450: Modify private methods in DuplicateEmailTests #7748

Closed

Fix part of #7450: Modify private methods in DuplicateEmailTests #7749

Closed

gopivaibhav mentioned this issue Feb 23, 2022

Fix part of #7450: Replaced private methods in SuggestionServicesUnitTests #15001

Merged

4 tasks

Mohitbalwani26 mentioned this issue Feb 25, 2022

Fix part of #7450: Replace testing of private methods in BuildTests #15020

Merged

4 tasks

ayushjaink8 mentioned this issue Mar 2, 2022

Fix tests/methods that refer to private functions in Python inside core.domain.exp_services_test.py #15060

Merged

4 tasks

vojtechjelinek pushed a commit that referenced this issue Mar 16, 2022

Fix part of #7450: Replaced private methods in SuggestionServicesUnit…

9c38bcc

…Tests (#15001) Co-authored-by: Gopi Vaibhav <gopivaibhav@pop-os.localdomain>

U8NWXD moved this to In Progress in Developer Workflow Team Aug 1, 2022

U8NWXD added this to Developer Workflow Team Aug 1, 2022

kevintab95 added the enhancement Label to indicate an issue is a feature/improvement label Aug 30, 2022

U8NWXD added Impact: Low -- ONLY DO IF GOOD FIRST ISSUE Postponing for now, since it doesn't affect users much. Work: Low Solution is known and broken into good-first-issue-sized chunks. and removed backend labels Oct 4, 2022

seanlip removed the good second issue label Dec 12, 2022

seanlip moved this from In Progress to Todo in Developer Workflow Team Dec 12, 2022

Fix tests/methods that refer to private functions in Python #7450

Fix tests/methods that refer to private functions in Python #7450

Comments

kevinlee12 commented Aug 24, 2019 • edited by vojtechjelinek Loading

ykabusalah commented Aug 30, 2019

kevinlee12 commented Aug 31, 2019

lilithxxx commented Sep 2, 2019

kevinlee12 commented Sep 2, 2019

xoxwaw commented Sep 25, 2019 • edited Loading

kevinlee12 commented Sep 26, 2019

xoxwaw commented Sep 27, 2019

kevinlee12 commented Sep 27, 2019

KristiKovacs commented Oct 3, 2019

DubeySandeep commented Oct 3, 2019

rahmot commented Oct 4, 2019

DubeySandeep commented Oct 4, 2019

rahmot commented Oct 4, 2019

DubeySandeep commented Oct 4, 2019 • edited Loading

rahmot commented Oct 4, 2019

Hudda commented Oct 5, 2019

DubeySandeep commented Oct 5, 2019 • edited Loading

Hudda commented Oct 5, 2019

DubeySandeep commented Oct 5, 2019

vojtechjelinek commented Feb 14, 2022

gopivaibhav commented Feb 15, 2022

vojtechjelinek commented Feb 15, 2022

Mohitbalwani26 commented Feb 15, 2022

vojtechjelinek commented Feb 15, 2022

Mohitbalwani26 commented Feb 24, 2022

vojtechjelinek commented Feb 24, 2022

ayushjaink8 commented Feb 25, 2022

vojtechjelinek commented Feb 25, 2022

Mohitbalwani26 commented Feb 27, 2022

vojtechjelinek commented Feb 27, 2022

jordyparker commented Feb 27, 2022

JeeveshGarg commented Jul 15, 2022

vojtechjelinek commented Jul 15, 2022

JeeveshGarg commented Jul 16, 2022 • edited Loading

U8NWXD commented Jul 24, 2022

kevinlee12 commented Aug 24, 2019 •

edited by vojtechjelinek

Loading

xoxwaw commented Sep 25, 2019 •

edited

Loading

DubeySandeep commented Oct 4, 2019 •

edited

Loading

DubeySandeep commented Oct 5, 2019 •

edited

Loading

JeeveshGarg commented Jul 16, 2022 •

edited

Loading