Show first and last node ctime for verdi storage info --detailed

[ayushjariyal]

issue #6817

Updating the code for additional information to be shown with verdi storage info --detailed by adding the creation time (ctime) and modification time (mtime) of the first and last nodes in the database.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.27%. Comparing base (ae65e2d) to head (8c08491).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6829      +/-   ##
==========================================
+ Coverage   79.26%   79.27%   +0.01%     
==========================================
  Files         566      566              
  Lines       43794    43798       +4     
==========================================
+ Hits        34711    34715       +4     
  Misses       9083     9083              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ayushjariyal]

@GeigerJ2, Can you please review this PR?

[GeigerJ2]

Hi @ayushjariyal, I'm sorry I overlooked this and am just getting back here. Thank you for your contribution! I just rebased the PR to bring it to the current state of the code base. The implementation is good¹, could you also add tests for the new functionality? Otherwise, I'm also happy to finalize this (I know it's been long since you opened this). Cheers!

Footnotes

1. I noticed that the output of verdi storage info might have some additional problems (see verdi archive info CLI output unreadable with large entities_starting_set #6978), but this can probably be solved in a different PR. ↩

[ayushjariyal]

Hi @GeigerJ2 , thanks for reviewing and rebasing the PR! I’ll be happy to add the tests for the new functionality. Could you please point me to the appropriate test file or directory where these should be added?

[GeigerJ2]

Regarding the tests, there is a test of the get_info method (test_get_info) which is the only caller of the get_orm_entities method where you added your changes. Going through the callers / references of the relevant functions / methods at hand is a nice way to discover where they are being tested, bc they are actually being called in the tests :)

The test method is located in tests/storage/psql_dos/test_backend.py. Note that to actually run it locally, you need a working PostgreSQL setup on your machine, and use the --db-backend psql option of pytest¹. Hence, a command could look something like:

pytest tests/storage/psql_dos/test_backend.py::test_get_info --db-backend psql

You can also add the -s option to allow entering the debugger, if you add a breakpoint in the test. The method uses a get_mock_info to mock the get_info of the DiskObjectStoreRepositoryBackend, but for the ORM backend, the method you modified is called. Then, you can assert that the output you added and expect is present.

Footnotes

1. These also seem like tests that were historically implemented for PSQL, but don't have to be anymore, so could, in principle, also be run with SQLite, @danielhollas? I just copied this test over to sqlite_dos backend, and it worked the same. ↩

[danielhollas]

These also seem like tests that were historically implemented for PSQL, but don't have to be anymore, so could, in principle, also be run with SQLite, @danielhollas?

Absolutely! When I was first introducing sqlite-based tests I took a shortcut and automatically marked all tests in tests/storage/psql_dos/ with requires_psql marker. Would you mind opening an issue for this? We should audit all tests in that directory and move any that are backend-agnostic. I'd definitely do that in another PR though.

[GeigerJ2]

@ayushjariyal, do you have time to work on this? Otherwise, I can also add the tests and we share authorship on the PR? Would like to wrap it up soon :)

[ayushjariyal]

@GeigerJ2 , I’m a bit caught up with college work right now, so please go ahead and add the tests. Happy to share authorship on the PR—thanks a lot for helping wrap this up!

[GeigerJ2]

@danielhollas, maybe you can give this a quick review? Should be simple enough. While this is technically a new feature, and could go into 2.8.0, I think the changes are minor enough that we can put it into 2.7.2. An ASCII histogram of node creation time as written in #6817 (and brought up by Giovanni) seems a bit overkill for now, there are more pressing things to work on.

[GeigerJ2]

OK, change of mind. Moved this to 2.8.0. New feature, minor release. Let's follow SemVer properly...

[danielhollas]

OK, change of mind. Moved this to 2.8.0. New feature, minor release. Let's follow SemVer properly...

Haha, I was gonna write something, now I don't have to (see comment on Discourse :-) ).
But I'll give this a look and we can merge!

[danielhollas]

Passing None if there are no Nodes results in the following output from verdi storage info --detailed

  Nodes:
    count: 0
    first_created: null
    last_created: null
    node_types: []
    process_types: []

an alternative would be to pass en empty string, which would then look like this:

  Nodes:
    count: 0
    first_created: ''
    last_created: ''
    node_types: []
    process_types: []

I think None makes sense here. cc @GeigerJ2

[GeigerJ2]

Yes, agreed, I think null is fine :3
As always, thanks for the review, @danielhollas 🫶

[danielhollas]

I tweaked the test and tested locally, looks great, thanks @ayushjariyal! 🙏