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

feat: support installing git scripts from non-root folders #911

Merged
merged 10 commits into from Jul 23, 2022

Conversation

midgleyc
Copy link
Member

Certain existing scripts, such as ChIT install from a folder one level lower than the base directory when using SVN. It is desirable to support this from git as well, to avoid breaking existing installations.

Introduce a file manifest.json with a key root_directory to specify the directory that contains the other files Mafia is interested in: those under the permissible folders (e.g. scripts), and the dependencies.txt file.

Removed comment about not supporting local changes because local changes are supported implicitly without me having to write any code, which is nice. It seems that git update will handle unstaged changes -- I thought asking people to commit changes was a good halfway house, but even this isn't necessary. Merge conflicts will probably error, though.

To install ChIT locally, a user could:

  • run git checkout https://github.com/Loathing-Associates-Scripting-Society/ChIT.git to install ChIT, but not copy any files
  • create a manifest.json file under MAFIA_ROOT/git/Loathing-Associates-Scripting-Society-ChIT containing
{
  "root_directory": "src"
}
  • run git sync

This file could be added to the remote git repo, after this feature is released, to avoid the need for local changes.

I'm not certain I want the configuration to be json or to be named as it is -- arguments for / against welcomed.

@midgleyc midgleyc requested a review from a team as a code owner July 20, 2022 18:20
@gausie
Copy link
Contributor

gausie commented Jul 20, 2022

👍 for manifest.json

@codecov
Copy link

codecov bot commented Jul 20, 2022

Codecov Report

Merging #911 (95bad18) into main (101b0ac) will increase coverage by 0.29%.
The diff coverage is 55.10%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #911      +/-   ##
============================================
+ Coverage     26.43%   26.72%   +0.29%     
- Complexity    12240    12370     +130     
============================================
  Files          1028     1028              
  Lines        159692   159705      +13     
  Branches      35132    35132              
============================================
+ Hits          42221    42688     +467     
+ Misses       109710   109144     -566     
- Partials       7761     7873     +112     
Impacted Files Coverage Δ
...t/sourceforge/kolmafia/scripts/git/GitManager.java 59.07% <55.10%> (+59.07%) ⬆️
...rceforge/kolmafia/persistence/HolidayDatabase.java 34.57% <0.00%> (ø)
src/net/sourceforge/kolmafia/KoLCharacter.java 49.93% <0.00%> (+0.04%) ⬆️
...forge/kolmafia/persistence/ConcoctionDatabase.java 46.21% <0.00%> (+0.05%) ⬆️
src/net/sourceforge/kolmafia/KoLmafiaCLI.java 68.54% <0.00%> (+0.15%) ⬆️
...et/sourceforge/kolmafia/textui/RuntimeLibrary.java 34.24% <0.00%> (+0.33%) ⬆️
src/net/sourceforge/kolmafia/RequestLogger.java 17.10% <0.00%> (+0.41%) ⬆️
src/net/sourceforge/kolmafia/textui/DataTypes.java 49.21% <0.00%> (+1.34%) ⬆️
src/net/sourceforge/kolmafia/RequestThread.java 27.87% <0.00%> (+1.81%) ⬆️
...urceforge/kolmafia/scripts/svn/WCEventHandler.java 4.76% <0.00%> (+4.76%) ⬆️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 101b0ac...95bad18. Read the comment docs.

@gausie
Copy link
Contributor

gausie commented Jul 21, 2022

Would you be up for adding more tests to GitManager?

@midgleyc
Copy link
Member Author

I think it's feasible to add some, with downside that:

  • it requires an external git repo (I've been using https://github.com/midgleyc/mafia-script-install-test), which requires a network connection. I might be able to use a created local repo somehow?
  • it uses the real filesystem (have to be careful to clean up after)
  • you can't test (or at least I can't think of a way to test) the really complicated bits (e.g. that git update can delete files successfully. This is easy manually.)
  • it's way easier to test manually than it is to write the unit tests

I would be looking to go through the CLI and ASH commands, instead of testing the manager, because I don't want to be tied to the current public interface.

@gausie
Copy link
Contributor

gausie commented Jul 22, 2022

I think it's feasible to add some, with downside that:

* it requires an external git repo (I've been using https://github.com/midgleyc/mafia-script-install-test), which requires a network connection. I might be able to use a created local repo somehow?

I wouldn't mind keeping an example git repo in the tests directory

* it uses the real filesystem (have to be careful to clean up after)

Yeah I guess alternatively we mock the filesystem layer but I appreciate that's tedious.

* you can't test (or at least I can't think of a way to test) the really complicated bits (e.g. that `git update` can delete files successfully. This is easy manually.)

Mocking the fs I suppose this would be doable.

@midgleyc
Copy link
Member Author

If I were to avoid the remote network, I think the way would be:

  • in the test, use JGit to create a local repo on the filesystem
  • figure out what URI to pass JGit to clone that repo (file://<path>?)
  • test accordingly

As an increment, though, I think I can create some branches on my test repo and use that.

@midgleyc midgleyc force-pushed the git-manifest branch 2 times, most recently from c3cecd4 to f467166 Compare July 22, 2022 16:18
@jaadams5
Copy link
Contributor

FWIW, DebugDatabaseTest mocks a "simple" and "complex" file system. I'm not sure how relevant it is because the focus is in files and attributes. To test git commands you'd also need file contents, but maybe this would be extendable? MoodManagerTest mocks reading a text file. Probably irrelevant but sometimes even a bad example can be inspirational.

@midgleyc
Copy link
Member Author

I saw that: that creates a fake File and passes it to a few functions. I don't think mocking the filesystem the same way in GitManager is feasible because of the entrypoints. For example:

  • checkout takes a URI
  • list looks in a directory for filenames recursively
  • update takes a path to a full git repository

Mocking any of these would be extremely fragile and tightly coupled to the implementation, and wouldn't give high confidence the code actually worked. I feel like it's a non-starter.

Copy link
Contributor

@gausie gausie left a comment

Choose a reason for hiding this comment

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

This is great! Thank you

@midgleyc midgleyc merged commit 6f53bd8 into kolmafia:main Jul 23, 2022
@midgleyc midgleyc deleted the git-manifest branch July 23, 2022 15:08
@midgleyc midgleyc added the git-scripts Install scripts using git label Aug 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
git-scripts Install scripts using git
Projects
None yet
3 participants