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

<DataNode>.is_up_to_date raises error when never written before #1224

Merged
merged 6 commits into from
May 3, 2024
Merged

<DataNode>.is_up_to_date raises error when never written before #1224

merged 6 commits into from
May 3, 2024

Conversation

yaten2302
Copy link
Contributor

Fixes #1198

@trgiangdo
Copy link
Member

Hello @yaten2302, thank you for your contribution.

Can you add a unittest that fails before your fix, and passes after your fix?

@trgiangdo trgiangdo added 💥Malfunction Addresses an identified problem. Core: Data node labels Apr 24, 2024
@yaten2302
Copy link
Contributor Author

Hey @trgiangdo, actually, I'm not really familiar on how to add tests. Could you please guide that how can I create and add the same?

@trgiangdo
Copy link
Member

@yaten2302 You can take a look at https://github.com/Avaiga/taipy/blob/c3aaf92e767c0844c698701a9c18d15d19bf60ba/tests/core/data/test_data_node.py.
Your test maybe similar to the tests in this file.

Basically, the test should:

  • Create a DataNode entity (InMemoryDataNode or PickleDataNode for example)
  • Try to assert if the dn.is_up_to_date attribute is False. Before your fix, this will raise an error.

@yaten2302
Copy link
Contributor Author

@yaten2302 You can take a look at https://github.com/Avaiga/taipy/blob/c3aaf92e767c0844c698701a9c18d15d19bf60ba/tests/core/data/test_data_node.py. Your test maybe similar to the tests in this file.

Basically, the test should:

  • Create a DataNode entity (InMemoryDataNode or PickleDataNode for example)
  • Try to assert if the dn.is_up_to_date attribute is False. Before your fix, this will raise an error.

Do I've to do like this? In this file: https://github.com/Avaiga/taipy/blob/c3aaf92e767c0844c698701a9c18d15d19bf60ba/tests/core/data/test_data_node.py

class TestData:
   def testDataNode:
       dn = InMemoryDataNode("dn", Scope.SCENARIO)
       assert dn.is_up_to_date == False

@trgiangdo
Copy link
Member

Yes, and you can also try to write to the data node, then assert dn.is_up_to_date == True since it should be True then

@trgiangdo trgiangdo self-requested a review April 26, 2024 04:52
@yaten2302
Copy link
Contributor Author

Thanks, @trgiangdo. I'll make the changes by today. Also, could you please tell that if we want to test the changes, then do we've to run the command - pipenv run pytest?

@yaten2302
Copy link
Contributor Author

yaten2302 commented Apr 26, 2024

Hey @trgiangdo, I've put this code in the file tests/core/data/test_data_node.py. After that when I run the pipenv run pytest command, I'm getting this. Ig this means failure in a test right? Could you please guide that how can this be fixed?

image

@trgiangdo
Copy link
Member

Seems like the modification you made causes the tests/core/data/test_data_node.py::TestDataNode::test_is_up_to_date_across_scenarios() and tests/core/data/test_data_node.py::TestDataNode::test_is_up_to_date() to failed.

The reason should be shown the terminal when you run the test. You can run the specific test by running the pytest <<path_to_test_file>> to run the 1 failing file only.

@yaten2302
Copy link
Contributor Author

@trgiangdo, when I'm running the command pytest test_data_node.py, it's showing this error.

image
I've checked the dir as well, the test_data_node.py file is located in the taipy/tests/core/data dir only. Could you please guide that how can this be fixed?

@trgiangdo
Copy link
Member

You are not in the virtualenv. Please run pytest with the "pipenv run ..." or activate the virtualenv

@yaten2302
Copy link
Contributor Author

It's again showing the same error @trgiangdo

image
This time it created a Pipfile as well in the tests/core/data dir.

@trgiangdo
Copy link
Member

It's because you are running the pipenv command in a different directory. You need to cd back to the taipy folder where you created the previous virtualenv and run the test

@yaten2302
Copy link
Contributor Author

@trgiangdo, but when I'm running this command in the taipy folder, then it's showing the error that the file test_data_node.py not found.

image

@trgiangdo
Copy link
Member

It's because the path you provided after the pipenv run pytest <<path>> is wrong. You need to provide the full path.

@yaten2302
Copy link
Contributor Author

Yes, now it's working 👍 @trgiangdo. It's showing error in 2 functions - test_is_up_to_date (which I created) and test_is_up_to_date_across_scenarios (which was already present).

image


image
Could you please guide that how can these be fixed?

@trgiangdo
Copy link
Member

You modified the .is_up_to_date attribute so that it will be False when the data node is not written. I don't see the data node is written in those tests, am I right?

@yaten2302
Copy link
Contributor Author

Yes, @trgiangdo , what my test does is that it'll return False if the data node is not written.

@trgiangdo
Copy link
Member

Then why are you testing if it is up to date with the assert dn.is_up_to_date?

@yaten2302
Copy link
Contributor Author

@yaten2302 You can take a look at https://github.com/Avaiga/taipy/blob/c3aaf92e767c0844c698701a9c18d15d19bf60ba/tests/core/data/test_data_node.py. Your test maybe similar to the tests in this file.

Basically, the test should:

  • Create a DataNode entity (InMemoryDataNode or PickleDataNode for example)
  • Try to assert if the dn.is_up_to_date attribute is False. Before your fix, this will raise an error.

Do I've to do like this? In this file: https://github.com/Avaiga/taipy/blob/c3aaf92e767c0844c698701a9c18d15d19bf60ba/tests/core/data/test_data_node.py

class TestData:
   def testDataNode:
       dn = InMemoryDataNode("dn", Scope.SCENARIO)
       assert dn.is_up_to_date == False

I've added the data node in the test, but it's showing the error(ss I've sent in the above comment)

@trgiangdo
Copy link
Member

Then, I would add this test to the tests/core/data/test_data_node.py, right above the 2 tests that are failing.

    def test_is_up_to_date_when_not_written(self):
        dn_confg_1 = Config.configure_in_memory_data_node("dn_1", default_data="a")
        dn_confg_2 = Config.configure_in_memory_data_node("dn_2")
        task_config_1 = Config.configure_task("t1", funct_a_b, [dn_confg_1], [dn_confg_2])
        scenario_config = Config.configure_scenario("sc", [task_config_1])

        scenario = tp.create_scenario(scenario_config)

        assert scenario.dn_1.is_up_to_date is True
        assert scenario.dn_2.is_up_to_date is False

        tp.submit(scenario)
        assert scenario.dn_1.is_up_to_date is True
        assert scenario.dn_2.is_up_to_date is True

The idea is to have 2 data nodes:

  • the dn_1 has the default_data, which will be written to the data node right at the creation. Because of that, the scenario.dn_1.is_up_to_date should be True.
  • the dn_2 is only written when the scenario is submitted, so the scenario.dn_2.is_up_to_date should be False before the submission, and True after.

@trgiangdo trgiangdo self-assigned this Apr 30, 2024
@yaten2302
Copy link
Contributor Author

Hey @trgiangdo , sorry for inconvenience, actually when this issue was created, the code which I've copied was given in the description, and that's why I copied that. I'll change the code and add the tests as well, as told by you.

@trgiangdo
Copy link
Member

Thanks for your feedback.

The code in the issue description is just a prototype and not actually tested. The one who is assigned to the issue needs to handle the actual implementation and test the fix.

@yaten2302
Copy link
Contributor Author

Thanks for your feedback.

The code in the issue description is just a prototype and not actually tested. The one who is assigned to the issue needs to handle the actual implementation and test the fix.

Sure, I'll take care of this while contributing to other issues in this project 👍

@yaten2302
Copy link
Contributor Author

Also, I've added the code and the tests as well, it's working now 👍, only 1 test is getting failed (which was already present) - test_is_up_to_date_across_scenarios. Rest everything is working fine.

@trgiangdo trgiangdo marked this pull request as ready for review May 3, 2024 07:35
@trgiangdo trgiangdo self-requested a review May 3, 2024 07:35
@trgiangdo
Copy link
Member

What do you mean the failed test test_is_up_to_date_across_scenarios is already presented?
The test is not failing before and if it's failing now, it's due to the changes in your branch.

Also, there is a linter error:

tests/core/data/test_data_node.py:77:1: W293 [*] Blank line contains whitespace

where you left some redundant whitespaces at the end of the line.

@trgiangdo trgiangdo requested a review from jrobinAV May 3, 2024 07:59
@yaten2302
Copy link
Contributor Author

Sorry for the inconvenienced caused, ig the error was due to my mistake I accidentally left those white spaces in my code, I'll take care of it from the next time👍

Copy link
Member

@trgiangdo trgiangdo left a comment

Choose a reason for hiding this comment

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

I added 2 commits, 1 fix the whitespaces, and 1 fix the return in the wrong scope. The tests are all passing now.

Thank you again for your contribution.

Copy link
Member

@jrobinAV jrobinAV left a comment

Choose a reason for hiding this comment

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

Neat!
Thank you very much for your contribution.

@trgiangdo trgiangdo merged commit ab49ac5 into Avaiga:develop May 3, 2024
59 of 61 checks passed
@yaten2302 yaten2302 deleted the fix/#1198-DataNode.is_up_to_date-raises-error-when-never-written-before branch May 3, 2024 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core: Data node 💥Malfunction Addresses an identified problem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG- <DataNode>.is_up_to_date raises error when never written before
3 participants