UFRN-URNAI / urnai-tools

A modular Deep Reinforcement Learning library that supports multiple environments, made with Python 3.6.
Apache License 2.0
5 stars 8 forks source link

Resolves #81 #84

Closed CinquilCinquil closed 2 months ago

CinquilCinquil commented 5 months ago

81

CinquilCinquil commented 5 months ago

I saw that there was already a savable class on the previous version of URNAI, so i tested it, fixed some bugs and i think its ok to use now.

CinquilCinquil commented 5 months ago

For testing, i woud suggest first creating a class, that inherits from Savable, with some attributes. Then instantiate it, giving random values to the attributes, after that save it and load it to see if it still has the same values as before. I belive all methods of the class are used in this procedure, and that should reveal if any of them are not functional.

CinquilCinquil commented 5 months ago

Should i make unit tests?

alvarofpp commented 5 months ago

For testing, i woud suggest first creating a class, that inherits from Savable, with some attributes. Then instantiate it, giving random values to the attributes, after that save it and load it to see if it still has the same values as before. I belive all methods of the class are used in this procedure, and that should reveal if any of them are not functional.

The Savable class is not abstract, so you should use it directly in the tests. As these are unit tests, you should ideally create tests for each function separately.

alvarofpp commented 5 months ago

Should i make unit tests?

Yes, you should.

CinquilCinquil commented 5 months ago

What about the name Persistence for the class?

alvarofpp commented 5 months ago

What about the name Persistence for the class?

It's okay for now.

CinquilCinquil commented 4 months ago

Btw, my lint checks keep failing because of the description of an old commit (I put too many characters in a line apparently). Is there any way to fix that?

alvarofpp commented 4 months ago

Btw, my lint checks keep failing because of the description of an old commit (I put too many characters in a line apparently). Is there any way to fix that?

You can use interactive rebase (git rebase -i) for that.

alvarofpp commented 4 months ago

Unit tests are missing.

CinquilCinquil commented 3 months ago

In the _simple_save and load tests a file is created, but i wish to delete it after the test is done. I tried os.remove but it needs extra permission to work. Any suggestions?

alvarofpp commented 3 months ago

@CinquilCinquil I recommend mocking the functions that create and read the file, that way the file won't be created and won't need to be removed later.

CinquilCinquil commented 3 months ago

Oh, but the whole purpose of test_simple_save, for example, is to check if a file has been created. I'm not familiar with mocking, is there a way of faking file creation?

alvarofpp commented 3 months ago

Creating files is something that can result storage costs, and you have to make sure that each test involving _simple_save generates a different file so that you don't have the problem of trying to save to the same file or the file not existing because of another test. In short, it's something that can cause more problems than it solves. To test this, I believe that flow/process tests are more suitable than unit tests.

What you can do is mock the function that saves or reads the file and change its execution, in which case you should apply the mock to the return_value of the function.