[1LP][RFR] Create test_import_tag and implement fixtures and functions it needs by prichard77 · Pull Request #10162 · ManageIQ/integration_tests

prichard77 · 2020-06-03T04:02:37Z

Purpose or Intent

Adding test for importing tags from csv file to cover BZ 1792185.

PRT Run

{{ pytest: --use-provider vsphere65-nested cfme/tests/configure/test_tag.py::test_import_tag }}

…rting tags

prichard77 · 2020-06-05T04:19:00Z

I still might have some debugging to do in the csv file cleanup, but wanted to get the review started.

mshriver · 2020-06-05T12:51:53Z

cfme/tests/configure/test_tag.py


+@pytest.fixture
+def csv_tag_file(create_vm, category, tag):
+    file_name = 'test_import_' + fauxfactory.gen_alphanumeric(4) + ".csv"


I highly recommend using tempfile for this:
https://docs.python.org/3.7/library/tempfile.html

mshriver · 2020-06-05T14:42:57Z

cfme/tests/configure/test_tag.py


+@pytest.fixture
+def csv_tag_file(create_vm, category, tag):
+    temp_file = tempfile.NamedTemporaryFile(suffix=".csv")


So, you can use tempfiles's context manager here to make this even simpler.

with tempfile.NamedTemporaryFile(suffix='.csv') as temp_file: tempfile.write(csv_data) yield temp_file.name

Note this is yielding the file name, not the file object, which is what the test actually needs anyway.

After the yield, the context manager exists and the file is deleted

Updated as suggested. I was on the fence about returning the object or just name. I relaize I just use the name, but was thinking I might want to access the object if I used this fixture for another test. Using it in another test seems quite unlikely now.

getting error trying to use tempfile: TypeError: expected str, bytes or os.PathLike object, not _TemporaryFileWrapper . Investigating.

I was unable to get the shortened version to work. Tried a few things but was getting errors or creating empty files. Moving on to the asserts for the vm tag.

mshriver · 2020-06-05T14:53:25Z

cfme/configure/configuration/region_settings.py

+
+            Args:
+                file_name: Name of .csv file containing tag data.
+                By default file must be at cfme/tests/configure/


Why must the file be at this location? tempfile should take care of the file location, and it shouldn't matter where the file is for the import in the UI.

Removed the comment. his is where I was seeing the framework put the file. I guess it handles this location so we don't need to care about it.

mshriver · 2020-06-05T14:58:27Z

cfme/tests/configure/test_tag.py

+@pytest.fixture
+def csv_tag_file(create_vm, category, tag):
+    temp_file = tempfile.NamedTemporaryFile(suffix=".csv")
+    csv_data = f'name,category,entry\n{create_vm.name},{category.display_name},{tag.display_name}'


Shouldn't you be using a random alphanumeric for the tag, instead of trying to import a tag that matches an existing one?

The tag fixture that you're using here is a random tag that has already been created on the category.

yes. The import is to add the existing tag value to a VM. If the tag or category or VM do not exist, the upload fails. the terms here are confusing between a category, entry, tag etc. . I believe i have the bahavior ad it is in the BZ.

Can you add to the docblock for the test case the description that you're importing tag mappings to a VM, and not just tag definitions.

mshriver · 2020-06-05T15:00:24Z

cfme/tests/configure/test_tag.py

+    Bugzilla:
+        1792185
+    """
+    category.import_tag_from_file(csv_tag_file.name)


I think some additional assertions are necessary here - you have the tag and category display names, you should be able to instantiate a Tag and assert that it exists.

Look at something like:

assert category.tags.instantiate(display_name=tag.display_name).exists

If the tag does not exist the import will not work. The BZ issue was that an error gets displayed after "Upload" is clicked but before the apply. We are covering the BZ here. I guess I could add an assert that the tag exists on the VM?

gotcha, that makes more sense - please update the docblock with more detailed description of what's being tested.

Also correct about the additional assertion of the tag on the VM. You should be able to call tag in create_vm.get_tags, as the tag has an __eq__ implementation.

I put the assert against the vm tags in the category.import_tag_from_file() method.
I could have put it in the test case itself, but chose the method.
If I put it in the testcase I wouldn't need to pass down the extra 2 args to import_tag_from_file and could leave more flexibility in other tests to check results for an already existing tag (just comparing the display name strings) without having to create the tag object.
But how much more tagging tests are we going to write? And if we do write more, couldn't I just move the assert out then?

I moved the validation of the tag on the VM out into the testcase.

dajoRH · 2020-06-08T15:02:10Z

I detected some fixture changes in commit 0438283

The local fixture csv_tag_file is used in the following files:

cfme/tests/configure/test_tag.py
- test_import_tag

Please, consider creating a PRT run to make sure your fixture changes do not break existing usage 😃

cfme/tests/configure/test_tag.py

john-dupuy

One question, otherwise LGTM 👍

Begin work on test_import_tag and create navigation and view for impo…

87f5a36

…rting tags

dajoRH added lint-ok WIP labels Jun 3, 2020

prichard77 added 3 commits June 3, 2020 23:14

Implement upload_file as FileInput

70716bd

Add creation of csv file

a527410

Add usage of tag fixture and fix csv file format

c29f23f

prichard77 changed the title ~~[WIP]Begin work on test_import_tag and create navigation and view for impo…~~ [WIP] Create test_import_tag and implement fixtures and functions it needs Jun 5, 2020

prichard77 added 2 commits June 4, 2020 23:48

Add apply button and usage and use create_vm

54bb6fd

Move csv file creation into a fixture

43650ff

prichard77 changed the title ~~[WIP] Create test_import_tag and implement fixtures and functions it needs~~ [WIPTEST] Create test_import_tag and implement fixtures and functions it needs Jun 5, 2020

dajoRH added WIP-testing and removed WIP labels Jun 5, 2020

mshriver reviewed Jun 5, 2020

View reviewed changes

Use Tempfile in csv_tag_file fixture

3e390b1

prichard77 changed the title ~~[WIPTEST] Create test_import_tag and implement fixtures and functions it needs~~ [RFR] Create test_import_tag and implement fixtures and functions it needs Jun 5, 2020

dajoRH removed the WIP-testing label Jun 5, 2020

Reorder imports

bd33e8f

mshriver reviewed Jun 5, 2020

View reviewed changes

prichard77 added 3 commits June 5, 2020 13:48

Add to test_import_tag doc block

6e9f5f3

Add check that tag is added to VM

a3bbb5b

Move vm tag validation assertion out of method and into test case

0438283

prichard77 requested a review from mshriver June 9, 2020 16:19

john-dupuy reviewed Jun 16, 2020

View reviewed changes

cfme/tests/configure/test_tag.py Show resolved Hide resolved

john-dupuy reviewed Jun 16, 2020

View reviewed changes

john-dupuy added the new-test-or-feature label Jun 16, 2020

john-dupuy approved these changes Jun 17, 2020

View reviewed changes

john-dupuy changed the title ~~[RFR] Create test_import_tag and implement fixtures and functions it needs~~ [1LP][RFR] Create test_import_tag and implement fixtures and functions it needs Jun 17, 2020

jawatts approved these changes Jun 19, 2020

View reviewed changes

jawatts merged commit 5521cd8 into ManageIQ:master Jun 19, 2020

Conversation

prichard77 commented Jun 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose or Intent

PRT Run

Uh oh!

prichard77 commented Jun 5, 2020

Uh oh!

mshriver Jun 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dajoRH commented Jun 8, 2020

Uh oh!

Uh oh!

john-dupuy left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

prichard77 commented Jun 3, 2020 •

edited

Loading

mshriver Jun 5, 2020 •

edited

Loading