Skip to content

Commit

Permalink
Merge pull request #6342 from JohnThomson/cleanupAppearanceTests
Browse files Browse the repository at this point in the history 
Clean up AppearanceSettings tests that depend on old props (#6342)
  • Loading branch information
@andrew-polk
andrew-polk authored Mar 4, 2024
2 parents b7d801b + 0f98da1 commit 08eb0d9
Show file tree
Hide file tree
Showing 2 changed files with 65 additions and 49 deletions.
12 changes: 6 additions & 6 deletions src/BloomExe/Book/AppearanceSettings.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ public string FirstPossiblyOffendingCssFile
public bool IsInitialized { get; private set; }

// create an array of properties and fill it in
private PropertyDef[] propertyDefinitions = new PropertyDef[]
protected PropertyDef[] propertyDefinitions = new PropertyDef[]
{
// this one is special because it doesn't correspond to a CSS variable. Instead, we will copy the contents of named file as rules at the end of the CSS file.
// The default here is rarely if ever relevant. Usually a newly created instance will be initialized from a folder, and the default will be overwritten,
Expand Down Expand Up @@ -936,7 +936,7 @@ public object ChangeableSettingsForUI
}
}

abstract class PropertyDef
public abstract class PropertyDef
{
public string Name;
public dynamic DefaultValue;
Expand All @@ -954,9 +954,9 @@ public void SetDefault(dynamic prop)
public abstract string GetCssVariableDeclaration(dynamic property);
}

abstract class CssPropertyDef : PropertyDef { }
public abstract class CssPropertyDef : PropertyDef { }

class StringPropertyDef : PropertyDef
public class StringPropertyDef : PropertyDef
{
public StringPropertyDef(string name, string overrideGroup, string defaultValue)
{
Expand All @@ -971,7 +971,7 @@ public override string GetCssVariableDeclaration(dynamic property)
}
}

class CssStringVariableDef : CssPropertyDef
public class CssStringVariableDef : CssPropertyDef
{
public CssStringVariableDef(string name, string overrideGroup, string defaultValue = null)
{
Expand All @@ -991,7 +991,7 @@ public override string GetCssVariableDeclaration(dynamic property)
/// <summary>
/// variables that can be used in rules like ` .something { display: var(--cover-topic-show) }`
/// </summary>
class CssDisplayVariableDef : CssPropertyDef
public class CssDisplayVariableDef : CssPropertyDef
{
public string TrueValue;
public string FalseValue;
Expand Down
102 changes: 59 additions & 43 deletions src/BloomTests/Book/AppearanceSettingsTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@
using System.Collections.Generic;
using System.Dynamic;
using System.IO;
using System.Linq;
using Bloom.Book;
using BloomTemp;
using Newtonsoft.Json;
using NUnit.Framework;
using SIL.Extensions;
using SIL.IO;

namespace BloomTests.Book
Expand Down Expand Up @@ -35,29 +37,29 @@ public void NewDoesNotGetDefaultsForNulls()
[Test]
public void GetCssOwnPropsDeclaration_HasCorrectTitleFieldsVariableValues()
{
var appearance = new AppearanceSettings();
var appearance = new AppearanceSettingsTest();
// This comes from a default in the propertyDefinitions. If we switch to making the
// default unspecified, this test will need to change.
Assert.That(
appearance.GetCssOwnPropsDeclaration(),
Does.Contain($"--cover-title-L2-show: doShow-css-will-ignore-this-and-use-default")
Does.Contain($"--boolean-test-L2-show: doShow-css-will-ignore-this-and-use-default")
);
//appearance.Update(new { cover-title-L2-show = false, foo = "blah" });
appearance.UpdateFromJson("{\"cover-title-L2-show\":false}");
//appearance.Update(new { boolean-test-L2-show = false, foo = "blah" });
appearance.UpdateFromJson("{\"boolean-test-L2-show\":false}");
Assert.That(
appearance.GetCssOwnPropsDeclaration(),
Does.Contain("--cover-title-L2-show: none;")
Does.Contain("--boolean-test-L2-show: none;")
);
}

[Test]
public void GetCssOwnPropsDeclaration_WithBrandingAndXmatter_ProducesCorrectCss()
{
var appearance = new AppearanceSettings();
var appearance = new AppearanceSettingsTest();
dynamic brandingSettings = JsonConvert.DeserializeObject<ExpandoObject>(
@"{
""cover-title-L2-show"": false,
""cover-title-L3-show"": true,
""boolean-test-L2-show"": false,
""boolean-test-L3-show"": true,
""cover-topic-show"": false,
""cover-languageName-show"": false
}"
Expand Down Expand Up @@ -92,15 +94,15 @@ public void GetCssOwnPropsDeclaration_WithBrandingAndXmatter_ProducesCorrectCss(

Assert.That(
ownCss,
Does.Contain("--cover-title-L2-show: doShow-css-will-ignore-this-and-use-default;")
Does.Contain("--boolean-test-L2-show: doShow-css-will-ignore-this-and-use-default;")
);
Assert.That(ownCss, Does.Contain($"--cover-title-L3-show: none"));
Assert.That(ownCss, Does.Contain($"--boolean-test-L3-show: none"));

Assert.That(brandingCss, Does.Contain($"--cover-title-L2-show: none"));
Assert.That(brandingCss, Does.Contain($"--boolean-test-L2-show: none"));

Assert.That(
brandingCss,
Does.Contain("--cover-title-L3-show: doShow-css-will-ignore-this-and-use-default;")
Does.Contain("--boolean-test-L3-show: doShow-css-will-ignore-this-and-use-default;")
);
Assert.That(brandingCss, Does.Contain("--cover-topic-show: none;"));

Expand All @@ -113,27 +115,28 @@ public void GetCssOwnPropsDeclaration_WithBrandingAndXmatter_ProducesCorrectCss(
[Test]
public void GetCssOwnPropsDeclaration_ItemVisibility_ChildOverrides_UsesChildValue()
{
var collectionAppearance = new AppearanceSettings();
collectionAppearance.UpdateFromJson("{\"cover-title-L2-show\":false}");
var bookAppearance = new AppearanceSettings();
var collectionAppearance = new AppearanceSettingsTest();
collectionAppearance.UpdateFromJson("{\"boolean-test-L2-show\":false}");
var bookAppearance = new AppearanceSettingsTest();
bookAppearance.UpdateFromJson(
"{\"groupsToOverrideFromParent\":[\"coverFields\"], \"cover-title-L2-show\":true}"
"{\"groupsToOverrideFromParent\":[\"coverFields\"], \"boolean-test-L2-show\":true}"
);
Assert.IsTrue(
bookAppearance
.GetCssOwnPropsDeclaration(collectionAppearance)
.IndexOf($"--cover-title-L2-show: {AppearanceSettings.kDoShowValueForDisplay};")
> -1
.IndexOf(
$"--boolean-test-L2-show: {AppearanceSettings.kDoShowValueForDisplay};"
) > -1
);

bookAppearance.UpdateFromJson(
"{\"groupsToOverrideFromParent\":[\"coverFields\"],\"cover-title-L2-show\":false}"
"{\"groupsToOverrideFromParent\":[\"coverFields\"],\"boolean-test-L2-show\":false}"
);

Assert.IsTrue(
bookAppearance
.GetCssOwnPropsDeclaration(collectionAppearance)
.IndexOf($"--cover-title-L2-show: {AppearanceSettings.kHideValueForDisplay};")
.IndexOf($"--boolean-test-L2-show: {AppearanceSettings.kHideValueForDisplay};")
> -1,
bookAppearance.GetCssOwnPropsDeclaration(collectionAppearance).ToString()
);
Expand All @@ -142,29 +145,30 @@ public void GetCssOwnPropsDeclaration_ItemVisibility_ChildOverrides_UsesChildVal
[Test]
public void GetCssOwnPropsDeclaration_ItemVisibility_NoOverride_UsesParentValue()
{
var collectionAppearance = new AppearanceSettings();
var collectionAppearance = new AppearanceSettingsTest();

var bookAppearance = new AppearanceSettings();
var bookAppearance = new AppearanceSettingsTest();
bookAppearance.UpdateFromJson(
"{\"groupsToOverrideFromParent\":[], \"cover-title-L2-show\":true}"
"{\"groupsToOverrideFromParent\":[], \"boolean-test-L2-show\":true}"
);
collectionAppearance.UpdateFromJson("{\"cover-title-L2-show\":false}");
collectionAppearance.UpdateFromJson("{\"boolean-test-L2-show\":false}");
Assert.IsTrue(
bookAppearance
.GetCssOwnPropsDeclaration(collectionAppearance)
.IndexOf($"--cover-title-L2-show: {AppearanceSettings.kHideValueForDisplay};")
.IndexOf($"--boolean-test-L2-show: {AppearanceSettings.kHideValueForDisplay};")
> -1
);

collectionAppearance.UpdateFromJson("{\"cover-title-L2-show\":true}");
collectionAppearance.UpdateFromJson("{\"boolean-test-L2-show\":true}");
bookAppearance.UpdateFromJson(
"{\"groupsToOverrideFromParent\":[\"some-randome-thing\"],\"cover-title-L2-show\":false}"
"{\"groupsToOverrideFromParent\":[\"some-randome-thing\"],\"boolean-test-L2-show\":false}"
);
Assert.IsTrue(
bookAppearance
.GetCssOwnPropsDeclaration(collectionAppearance)
.IndexOf($"--cover-title-L2-show: {AppearanceSettings.kDoShowValueForDisplay};")
> -1,
.IndexOf(
$"--boolean-test-L2-show: {AppearanceSettings.kDoShowValueForDisplay};"
) > -1,
bookAppearance.GetCssOwnPropsDeclaration(collectionAppearance).ToString()
);
}
Expand Down Expand Up @@ -259,13 +263,13 @@ public void MayBeIncompatible_compatibleWithAppearanceVersion_False()
[Test]
public void ToCss_ContainsSettingsFromJson()
{
var settings = new AppearanceSettings();
var settings = new AppearanceSettingsTest();
settings.UpdateFromJson(
@"
{
""cssThemeName"": ""default"",
""cover-title-L2-show"": false,
""cover-title-L3-show"": true,
""boolean-test-L2-show"": false,
""boolean-test-L3-show"": true,
""cover-topic-show"": true,
""cover-languageName-show"": false
}"
Expand All @@ -275,10 +279,10 @@ public void ToCss_ContainsSettingsFromJson()
new string[] { "/* From this book's appearance settings */" },
StringSplitOptions.None
)[1];
Assert.That(fromSettings, Does.Contain("--cover-title-L2-show: none;"));
Assert.That(fromSettings, Does.Contain("--boolean-test-L2-show: none;"));
Assert.That(
fromSettings,
Does.Contain("--cover-title-L3-show: doShow-css-will-ignore-this-and-use-default;")
Does.Contain("--boolean-test-L3-show: doShow-css-will-ignore-this-and-use-default;")
);
Assert.That(
fromSettings,
Expand Down Expand Up @@ -328,7 +332,7 @@ public void OneTimeSetUp()
_tempFolder = new TemporaryFolder("GetThemeAndSubstituteCssSuccessTests");
_bookFolder = _tempFolder.Combine("book");
Directory.CreateDirectory(_bookFolder);
_settings = new AppearanceSettings();
_settings = new AppearanceSettingsTest();
var cssFilesToCheck = new[]
{
Tuple.Create(
Expand All @@ -345,7 +349,7 @@ public void OneTimeSetUp()
_settings.UpdateFromJson(jsonSettings.ToString());
_settings.WriteToFolder(_bookFolder);
_settings.WriteCssToFolder(_bookFolder);
_resultingAppearance = new AppearanceSettings();
_resultingAppearance = new AppearanceSettingsTest();
_resultingAppearance.UpdateFromFolder(_bookFolder);
_resultingAppearance.Initialize(
new[]
Expand Down Expand Up @@ -505,12 +509,6 @@ public void CssOfChosenTheme_OverridesMargin()
Assert.That(_cssOfEbookZeroMarginTheme, Does.Contain("--page-margin-right: 0mm;"));
}

[Test]
public void CssOfChosenTheme_DoesNotOverrideTitleVisibility()
{
Assert.That(_cssOfEbookZeroMarginTheme, Does.Not.Contain("--cover-title-L2-show:")); // not overridden in the efl-zero-margin-ebook theme
}

[Test]
public void CssOfSettingsObject_OverridesMargin()
{
Expand All @@ -524,8 +522,26 @@ public void CssOfSettingsObject_OverridesTitleVisibility()
// If we decide to support a "not specified" value, we'll need to change this test.
Assert.That(
_cssOfSettingsObject,
Does.Contain("--cover-title-L2-show: doShow-css-will-ignore-this-and-use-default")
Does.Contain("--boolean-test-L2-show: doShow-css-will-ignore-this-and-use-default")
);
}
}
}

/// <summary>
/// A class for testing the AppearanceSettings class. It is a subclass of AppearanceSettings so that
/// we can add a couple of spurious properties to it for testing. These properties replace the cover-title-LX-show
/// properties, which we don't want to count on in testing CssDisplayVariableDef, since we are no longer counting
/// on generated variables to control visibility of those fields, and may stop generating them altogether.
/// </summary>
public class AppearanceSettingsTest : AppearanceSettings
{
public AppearanceSettingsTest()
{
var testDef1 = new CssDisplayVariableDef("boolean-test-L2-show", "coverFields", true);
var testDef2 = new CssDisplayVariableDef("boolean-test-L3-show", "coverFields", false);
propertyDefinitions = propertyDefinitions.Concat(new[] { testDef1, testDef2, }).ToArray();
testDef1.SetDefault(_properties);
testDef2.SetDefault(_properties);
}
}

0 comments on commit 08eb0d9

Please sign in to comment.