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

WIP #846 Initial idea for unit tests for ModLibrary #919

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bencsikandrei
Copy link
Contributor

@bencsikandrei bencsikandrei commented Jan 27, 2024

hi, @lethal-guitar hope you're doing well!

I've noticed you closed this issue: #846. Why? I just came up with, I think, a cool idea to solve it!

I'm not there yet, but inching closer. Need to get some sleep soon (real late here) but if you approve of this idea, I'll absolutely go the extra mile!

Main points:

Add a fileshystem "stub" (handle?) for the filesystem functions used in ModLibrary::rescan
ideas: make a struct with function pointers, why?

  • simpler than virtual (yes, it is)
  • allows us to easily use std::filesystem::functions, no need for wrappers
  • we don't use gmock anyways, so mocking isn't as easy
  • simpler than virtual (yes, I really don't like 'em, that's why it's twice)

very veeeery much WIP, didn't test much apart from building the code and the unit test, didn't even have time to step through the debugger, it's really late, this commit message will change :)

  • [ Y] I've compiled the code locally on my machine, and it builds without errors
  • [ N] I've launched the game on my machine after making my changes, and I can successfully start a new game from the main menu (it's late don't have much time right now but wanted to just put this idea into writing)

@lethal-guitar what do you think? worth pursuing?

here's some logs from the current unit test (it's very basic of course):

andrew@pop-os:~/Projects/RigelEngine$ ./build/gcc-11-Debug/test/tests 
2024-01-27 23:27:28.900 (   0.000s) [        8FE553C0]        mod_library.cpp:127   INFO| { rescan
2024-01-27 23:27:28.900 (   0.000s) [        8FE553C0]        mod_library.cpp:131   INFO| .   Listing mod directories
2024-01-27 23:27:28.900 (   0.000s) [        8FE553C0]        mod_library.cpp:157   INFO| .   Found 0 mods
2024-01-27 23:27:28.900 (   0.000s) [        8FE553C0]        mod_library.cpp:162   INFO| .   No previous mod library, creating default selection
2024-01-27 23:27:28.900 (   0.000s) [        8FE553C0]        mod_library.cpp:127   INFO| } 0.000 s: rescan
===============================================================================
test cases: 1 | 1 passed
assertions: - none -

add a fileshystem stub for the filesystem functions used in
ModLibrary::rescan
ideas: make a struct with function pointers, why?
  * simpler than virtual (yes, it is)
  * allows us to easily use std::filesystem::functions, no need for
    wrappers
  * we don't use gmock anyways, so mocking isn't as easy
  * simpler than virtual (yes, I really don't like 'em, that's why it's
    twice)

very much WIP, didn't test much, it's really late, this commit message
will change :)
#include <catch.hpp>
RIGEL_RESTORE_WARNINGS

#include <cstdio>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

why did I leave this here?


Filesystem mockFsHandle{
[](const std::filesystem::path& p, std::error_code& ec) noexcept -> bool
{ return true; },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

foolish mocks, but I just wanted a proof of concept

Copy link
Contributor Author

Choose a reason for hiding this comment

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

even without Gmock we can mock these nicely, we can use some static arrays of booleans and iterate through them with a counter (similar to WillOnce/WillRepeteadely)

I think we can also hack the directory iteration function for all the calls we need (similarly with an array and counter) since we expect that one to be called twice - doable

test_spike_ball.cpp
test_string_utils.cpp
test_timing.cpp
# test_array_view.cpp
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note to self: so that I can compile faster, remove these when ready

@@ -21,6 +21,27 @@
#include <tuple>
#include <vector>

struct Filesystem
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note to self, move/rename when done, discuss with Nikolai

DirectoryIterateFuncT f_directory_iterate;
};

extern const Filesystem gNativeFilesystemHandle;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe we could extend this for other purposes

{
return false;
}

if (!entry.exists(err) || err)
if (!fsHandle.f_exists(entry, err) || err)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note to self: adapt to this code base's style guide (sorry! was in a rush for a Proof of Concept)

@lethal-guitar
Copy link
Owner

Hi @bencsikandrei, good to see you back and thanks for the PR! I closed the issue but it's still relevant, you're very welcome to work on it if you're interested. Basically, I recently did a bit of cleanup and closed a whole bunch of issues that I don't realistically see myself getting around to anytime soon - I don't really have much time for the project at the moment. But reviewing and discussing should be doable.

I'll have a closer look at the proposal when I have time, and let you know what I think!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants