-
Notifications
You must be signed in to change notification settings - Fork 185
fix: portable mode re-prompting and stray trusted-plugins.json in AppData (#658) #659
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
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
11b5d74
fix: portable mode re-prompting and stray trusted-plugins.json in App…
648c98e
chore: update plugin hashes [skip ci]
github-actions[bot] fb63746
Merge branch 'Development' into bugfix/658-portable-mode-persistence
Hirogen 2aca7c5
chore: update plugin hashes [skip ci]
github-actions[bot] File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
133 changes: 133 additions & 0 deletions
133
src/LogExpert.Tests/Dialogs/SettingsDialogPortableModeTests.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,133 @@ | ||
| using LogExpert.Core.Config; | ||
| using LogExpert.Core.Interfaces; | ||
| using LogExpert.Dialogs; | ||
|
|
||
| using Moq; | ||
|
|
||
| using NUnit.Framework; | ||
|
|
||
| using UIStrings = LogExpert.Resources; | ||
|
|
||
| namespace LogExpert.Tests.Dialogs; | ||
|
|
||
| /// <summary> | ||
| /// Regression tests for issue #658: populating the settings dialog from preferences set the | ||
| /// portable-mode checkbox programmatically, which fired the CheckedChanged handler and ran the | ||
| /// full activation flow — showing the "copy settings?" question dialog and (re)creating the | ||
| /// marker file — every time the dialog was opened while portable mode was active. | ||
| /// </summary> | ||
| [TestFixture] | ||
| public class SettingsDialogPortableModeTests | ||
| { | ||
| private string _testDataPath = null!; | ||
| private string _portableConfigDir = null!; | ||
|
|
||
| private sealed record FillPortableModeResult(bool Finished, Exception? Failure, bool PortableModeAfterFill, string? CheckBoxText); | ||
|
|
||
| [SetUp] | ||
| public void SetUp () | ||
| { | ||
| _testDataPath = Path.Join(Path.GetTempPath(), "LogExpertSettingsDialogTests", Guid.NewGuid().ToString()); | ||
| _portableConfigDir = Path.Join(_testDataPath, "configuration"); | ||
| } | ||
|
|
||
| [TearDown] | ||
| [System.Diagnostics.CodeAnalysis.SuppressMessage("Design", "CA1031:Do not catch general exception types", Justification = "Unit Test")] | ||
| public void TearDown () | ||
| { | ||
| if (Directory.Exists(_testDataPath)) | ||
| { | ||
| try | ||
| { | ||
| Directory.Delete(_testDataPath, recursive: true); | ||
| } | ||
| catch | ||
| { | ||
| // Ignore cleanup errors | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Constructs the dialog and calls FillPortableMode on a disposable STA thread so that a | ||
| /// regression (the CheckedChanged flow showing a modal question dialog again) surfaces as | ||
| /// a Join timeout instead of hanging the run. | ||
| /// </summary> | ||
| private FillPortableModeResult RunFillPortableMode (bool portableMode) | ||
| { | ||
| Mock<IConfigManager> configManager = new(); | ||
| _ = configManager.SetupGet(m => m.PortableConfigDir).Returns(_portableConfigDir); | ||
| _ = configManager.SetupGet(m => m.PortableModeSettingsFileName).Returns("portableMode.json"); | ||
|
|
||
| Preferences preferences = new() | ||
| { | ||
| PortableMode = portableMode, | ||
| }; | ||
|
|
||
| Exception? failure = null; | ||
| var portableModeAfterFill = !portableMode; | ||
| string? checkBoxText = null; | ||
|
|
||
| Thread worker = new(() => | ||
| { | ||
| try | ||
| { | ||
| using SettingsDialog dialog = new(preferences, null!, 0, configManager.Object); | ||
| dialog.FillPortableMode(); | ||
|
|
||
| portableModeAfterFill = dialog.Preferences.PortableMode; | ||
| checkBoxText = dialog.Controls.Find("checkBoxPortableMode", searchAllChildren: true).Single().Text; | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| failure = ex; | ||
| } | ||
|
Hirogen marked this conversation as resolved.
Dismissed
|
||
| }) | ||
| { | ||
| IsBackground = true, | ||
| }; | ||
| worker.SetApartmentState(ApartmentState.STA); | ||
| worker.Start(); | ||
| var finished = worker.Join(TimeSpan.FromSeconds(20)); | ||
|
|
||
| return new FillPortableModeResult(finished, failure, portableModeAfterFill, checkBoxText); | ||
| } | ||
|
|
||
| [Test] | ||
| [Description("Issue #658: opening the settings dialog with portable mode active must not prompt or touch files")] | ||
| public void FillPortableMode_PortableModeActive_PerformsNoSideEffects () | ||
| { | ||
| var result = RunFillPortableMode(portableMode: true); | ||
|
|
||
| Assert.That(result.Finished, Is.True, | ||
| "FillPortableMode must return without user interaction — a modal question dialog means the portable-mode activation flow ran again"); | ||
| Assert.Multiple(() => | ||
| { | ||
| Assert.That(result.Failure, Is.Null, $"FillPortableMode must not throw, but threw: {result.Failure}"); | ||
| Assert.That(Directory.Exists(_portableConfigDir), Is.False, | ||
| "Populating the dialog must not create the portable configuration directory or marker file"); | ||
| Assert.That(result.PortableModeAfterFill, Is.True, "Preferences must be left untouched"); | ||
| Assert.That(result.CheckBoxText, Is.EqualTo(UIStrings.SettingsDialog_UI_DeActivatePortableMode), | ||
| "The checkbox label must reflect the active portable mode state"); | ||
| }); | ||
| } | ||
|
|
||
| [Test] | ||
| [Description("Issue #658: populating the dialog with portable mode off must also be side-effect free")] | ||
| public void FillPortableMode_PortableModeInactive_PerformsNoSideEffects () | ||
| { | ||
| var result = RunFillPortableMode(portableMode: false); | ||
|
|
||
| Assert.That(result.Finished, Is.True, | ||
| "FillPortableMode must return without user interaction"); | ||
| Assert.Multiple(() => | ||
| { | ||
| Assert.That(result.Failure, Is.Null, $"FillPortableMode must not throw, but threw: {result.Failure}"); | ||
| Assert.That(Directory.Exists(_portableConfigDir), Is.False, | ||
| "Populating the dialog must not touch the portable configuration directory"); | ||
| Assert.That(result.PortableModeAfterFill, Is.False, "Preferences must be left untouched"); | ||
| Assert.That(result.CheckBoxText, Is.EqualTo(UIStrings.SettingsDialog_UI_ActivatePortableMode), | ||
| "The checkbox label must reflect the inactive portable mode state"); | ||
| }); | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
135 changes: 135 additions & 0 deletions
135
src/PluginRegistry.Tests/PluginValidatorConfigPersistenceTests.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,135 @@ | ||
| using System.Reflection; | ||
|
|
||
| using NUnit.Framework; | ||
|
|
||
| namespace LogExpert.PluginRegistry.Tests; | ||
|
|
||
| /// <summary> | ||
| /// Regression tests for issue #658: trusted-plugins.json must only ever be written to the | ||
| /// directory the validator was configured with. The trust configuration must be loaded | ||
| /// lazily (at Initialize or first use), never eagerly at class-load time — an eager load | ||
| /// wrote the default file to %APPDATA%/LogExpert before Initialize could point the | ||
| /// validator at the portable configuration directory. | ||
| /// | ||
| /// Limitation: these tests protect the lazy-load mechanism (removing it fails them with a | ||
| /// NullReferenceException), but they cannot detect an eager class-load write directly — | ||
| /// the type is already loaded by the time any in-process test runs, and an eager load | ||
| /// targets the real %APPDATA% path. Keep PluginValidator free of a static constructor and | ||
| /// of I/O in field initializers. | ||
| /// </summary> | ||
| [TestFixture] | ||
| public class PluginValidatorConfigPersistenceTests | ||
| { | ||
| private string _testDataPath = null!; | ||
| private string _configDir = null!; | ||
|
|
||
| private string _originalConfigDirectory = null!; | ||
| private string _originalConfigPath = null!; | ||
| private object? _originalConfig; | ||
|
|
||
| private static FieldInfo GetStaticField (string name) | ||
| { | ||
| var field = typeof(PluginValidator).GetField(name, BindingFlags.NonPublic | BindingFlags.Static); | ||
| Assert.That(field, Is.Not.Null, $"Expected private static field '{name}' on PluginValidator"); | ||
| return field!; | ||
| } | ||
|
|
||
| [SetUp] | ||
| public void SetUp () | ||
| { | ||
| _testDataPath = Path.Join(Path.GetTempPath(), "LogExpertConfigPersistenceTests", Guid.NewGuid().ToString()); | ||
| _configDir = Path.Join(_testDataPath, "activeConfig"); | ||
| _ = Directory.CreateDirectory(_configDir); | ||
|
|
||
| _originalConfigDirectory = (string)GetStaticField("_configDirectory").GetValue(null)!; | ||
| _originalConfigPath = (string)GetStaticField("_configPath").GetValue(null)!; | ||
| _originalConfig = GetStaticField("_trustedPluginConfig").GetValue(null); | ||
| } | ||
|
|
||
| [TearDown] | ||
| [System.Diagnostics.CodeAnalysis.SuppressMessage("Design", "CA1031:Do not catch general exception types", Justification = "Unit Test")] | ||
| public void TearDown () | ||
| { | ||
| GetStaticField("_configDirectory").SetValue(null, _originalConfigDirectory); | ||
| GetStaticField("_configPath").SetValue(null, _originalConfigPath); | ||
| GetStaticField("_trustedPluginConfig").SetValue(null, _originalConfig); | ||
|
|
||
| if (Directory.Exists(_testDataPath)) | ||
| { | ||
| try | ||
| { | ||
| Directory.Delete(_testDataPath, recursive: true); | ||
| } | ||
| catch | ||
| { | ||
| // Ignore cleanup errors | ||
| } | ||
|
Hirogen marked this conversation as resolved.
Dismissed
|
||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Puts the validator into the state it is in at process start before Initialize has | ||
| /// run: no configuration loaded, config path pointing at the (here: sandboxed) default. | ||
| /// </summary> | ||
| private void SimulatePreInitializeState () | ||
| { | ||
| GetStaticField("_configDirectory").SetValue(null, _configDir); | ||
| GetStaticField("_configPath").SetValue(null, Path.Join(_configDir, "trusted-plugins.json")); | ||
| GetStaticField("_trustedPluginConfig").SetValue(null, null); | ||
| } | ||
|
|
||
| [Test] | ||
| [Description("Issue #658: config must be loaded lazily on first use, not eagerly at class load")] | ||
| public void ValidatePlugin_WhenConfigNotYetLoaded_LoadsLazilyFromConfiguredDirectory () | ||
| { | ||
| SimulatePreInitializeState(); | ||
|
|
||
| var pluginPath = Path.Join(_testDataPath, "SomePlugin.dll"); | ||
| File.WriteAllText(pluginPath, "not a real assembly"); | ||
|
|
||
| Assert.DoesNotThrow(() => PluginValidator.ValidatePlugin(pluginPath), | ||
| "ValidatePlugin must work without an eagerly loaded configuration"); | ||
|
|
||
| Assert.That(File.Exists(Path.Join(_configDir, "trusted-plugins.json")), Is.True, | ||
| "Lazy load should create the default configuration in the configured directory"); | ||
| } | ||
|
|
||
| [Test] | ||
| [Description("Issue #658: AddTrustedPlugin must lazily load and persist to the configured directory")] | ||
| public void AddTrustedPlugin_WhenConfigNotYetLoaded_PersistsToConfiguredDirectory () | ||
| { | ||
| SimulatePreInitializeState(); | ||
|
|
||
| var pluginPath = Path.Join(_testDataPath, "UserPlugin.dll"); | ||
| File.WriteAllText(pluginPath, "user plugin content"); | ||
|
|
||
| var result = PluginValidator.AddTrustedPlugin(pluginPath, out var errorMessage); | ||
|
|
||
| Assert.Multiple(() => | ||
| { | ||
| Assert.That(result, Is.True, $"AddTrustedPlugin should succeed, but failed with: {errorMessage}"); | ||
| Assert.That(File.Exists(Path.Join(_configDir, "trusted-plugins.json")), Is.True, | ||
| "Trust decision must be persisted to the configured directory"); | ||
| }); | ||
| } | ||
|
|
||
| [Test] | ||
| [Description("Normal mode is unchanged: Initialize creates the default config file in the given directory")] | ||
| public void Initialize_WithEmptyDirectory_CreatesDefaultConfigFileThere () | ||
| { | ||
| SimulatePreInitializeState(); | ||
|
|
||
| var freshDir = Path.Join(_testDataPath, "freshConfig"); | ||
| _ = Directory.CreateDirectory(freshDir); | ||
|
|
||
| PluginValidator.Initialize(freshDir); | ||
|
|
||
| Assert.Multiple(() => | ||
| { | ||
| Assert.That(File.Exists(Path.Join(freshDir, "trusted-plugins.json")), Is.True, | ||
| "Initialize should create the default configuration in its directory"); | ||
| Assert.That(File.Exists(Path.Join(_configDir, "trusted-plugins.json")), Is.False, | ||
| "Nothing may be written to the pre-Initialize default directory"); | ||
| }); | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.