Patches

Developer
Jan 3, 2011 at 3:02 PM

I have submitted a couple of patches and I think they have been declined but I can't seem to find out why. Could someone let me know how to find out why?

Coordinator
Jan 3, 2011 at 3:13 PM

It's in the descriptions of the patch declines.

The first one about the icon: Several problems in this patch (look at the source code diff).

  1. The .vcproj file now references htmllayout.dll, why?
  2. The .rc file versioning is trashed. DNI is generating a version number, then embedding it via the .rc. You can't re-edit the .rc with visual studio because it re-adds a VS_VERSION_INFO VERSIONINFO section.
  3. It would be nice to have a unit test for this (btw, because of (2) several unit tests will fail). 

The second one for the os filter.

  • The code is good, but I can't take a patch like this without unit tests. Who knows what existing functionality broke?! Extend existing and add more tests in UnitTests\dotNetInstallerToolsLibUnitTests\OsUtilUnitTests.cpp.

Hope this makes sense.

Thx,

-dB.

 

Developer
Jan 3, 2011 at 3:26 PM

No problem I just couldn't see how to find this information, without knowing this it makes it hard to fix the issues.

Developer
Jan 3, 2011 at 5:11 PM

I have just been working on the unit tests for my code and noticed that the current source fails a couple of tests:

1. Test name: DVLib::UnitTests::FormatUnitTests::testFormatDateTime    assertion failed    - Expression: "1969" == DVLib::FormatDateTimeA(0, "%Y")

On my system (Win 7 64 bit) this returns 1970.

2. Test name: DVLib::UnitTests::OsUtilUnitTests::testGetOperatingSystemLCIDassertion failed- Expression: lcid_system == lcid_userexe

lcid_system = 2057
lcid_userexe = 1033

Should these be failing or does it indicate an issue with my build environment?

 

Coordinator
Jan 3, 2011 at 6:35 PM

I think that the tests have issues, some have been written in the United States :) Do look into them and propose fixes! Thx.

Developer
Jan 3, 2011 at 9:42 PM

The problem is that none of the tests have comments so it is impossible to know what the aim of them is. So for problem 1 simply changing to 1970 fixes it but is that correct? For problem 2 I removed it as it doesn't work. So my patch would be:

void OsUtilUnitTests::testGetOperatingSystemLCID()
// CPPUNIT_ASSERT(lcid_system == lcid_userexe);

void FormatUnitTests::testFormatDateTime()
CPPUNIT_ASSERT("1970" == DVLib::FormatDateTimeA(0, "%Y")); // Was 1969
CPPUNIT_ASSERT(L"1970" == DVLib::FormatDateTimeW(0, L"%Y")); // Was 1969

 

Coordinator
Jan 3, 2011 at 10:56 PM

The testGetOperatingSystemLCID is dumb. We still need to check that lcid_userexe returns != 0, but otherwise yes.

The time test should be rewritten to not be timezone dependent. Try this:

	CPPUNIT_ASSERT(ERROR_SUCCESS == _putenv_s( "TZ", "UTC" ));
	_tzset();
	CPPUNIT_ASSERT(DVLib::FormatDateTimeA(0) == "1970-01-01 00:00:00");
	CPPUNIT_ASSERT("1970" == DVLib::FormatDateTimeA(0, "%Y"));
	CPPUNIT_ASSERT(DVLib::FormatDateTimeW(0) == L"1970-01-01 00:00:00");
	CPPUNIT_ASSERT(L"1970" == DVLib::FormatDateTimeW(0, L"%Y"));

I am in EST-5. If this works for you, I'll commit it. 

Most of these tests are trying to be self-explanatory, but I can see how it's not obvious :) Shame on me. Feel free to do better as you write new ones or edit existing tests! 

Developer
Jan 4, 2011 at 7:18 AM

Yes that test works for me know - I am GMT/UTC+0.

I have changed the testGetOperatingSystemLCID to:
CPPUNIT_ASSERT(lcid_userexe != 0);

I am not sure whether the test "lcid_system == lcid_user" is valid, it works on my PC but I can see that if I set the UI to be another langauge then it might fail.

By the way I agree about the testGetUpgradeCodes(), I raised it then immediately figured out what the issue was.

 

Coordinator
Jan 4, 2011 at 12:39 PM

Comitted @ rev. 59619. Thanks for your help.

Developer
Jan 4, 2011 at 1:07 PM

I have got the C++ unit tests working now but the nunit tests are failing:

trunk\dni.proj(200,32): error MSB4064: The "Force32Bit" parameter is not supported by the "NUnit" task. Verify the parameter exists on the task, and it is a settable public instanceproperty.

It seems the Force32Bit is not part of the msbuild community tasks - is this a known problem? Do you know a workaround?

Thanks, Neil.

Coordinator
Jan 4, 2011 at 1:08 PM

Maybe it was added in a newer MSBuild CT version?

Try the latest from http://msbuildtasks.tigris.org/MSBuild.Community.Tasks.Nightly.msi, if it works I'll update my public link.

Developer
Jan 4, 2011 at 1:21 PM

Yes that was it, the Force32bit parameter was added 2010-03-25. The nunit tests are running now but seem to be failing due to lcid errors:

        1) Test Failure : dotNetInstallerUnitTests.LanguageUnitTests.TestComponentLcid             Expected: True          But was:  False
        at dotNetInstallerUnitTests.LanguageUnitTests.TestComponentLcid() in d:\PAXTON\Net10\trunk\Release\dotNetInstaller 2.0\trunk\UnitTests\dotNetInstallerUnitTests\LanguageUnitTests.cs:line 52
        2) Test Failure : dotNetInstallerUnitTests.LanguageUnitTests.TestConfigNoLangID             Expected: 2057          But was:  1033
        at dotNetInstallerUnitTests.LanguageUnitTests.TestConfigNoLangID() in d:\PAXTON\Net10\trunk\Release\dotNetInstaller 2.0\trunk\UnitTests\dotNetInstallerUnitTests\LanguageUnitTests.cs:line 137
        3) Test Failure : dotNetInstallerUnitTests.LanguageUnitTests.TestConfigurationLcid             Expected: True          But was:  False
        at dotNetInstallerUnitTests.LanguageUnitTests.TestConfigurationLcid() in d:\PAXTON\Net10\trunk\Release\dotNetInstaller 2.0\trunk\UnitTests\dotNetInstallerUnitTests\LanguageUnitTests.cs:line 99
        4) Test Failure : dotNetInstallerUnitTests.LanguageUnitTests.TestOsLangID             Expected: 2057          But was:  1033

I'll see if I can work out why and submit a patch.

Developer
Jan 5, 2011 at 6:33 PM

I have a fix for the language unit tests but am not sure if it complies with what you are trying to test. I think the test should be using CultureInfo.CurrentUICulture.LCID not CultureInfo.CurrentCulture.LCID, if I make the changes the tests now work on my UK English system.

 

Index: LanguageUnitTests.cs
===================================================================
--- LanguageUnitTests.cs	(revision 59706)
+++ LanguageUnitTests.cs	(working copy)
@@ -24,7 +24,7 @@
             // current lcid
             string currentLcidFilename = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString());
             ComponentCmd cmdCurrentLcid = new ComponentCmd();
-            cmdCurrentLcid.os_filter_lcid = CultureInfo.CurrentCulture.LCID.ToString();
+            cmdCurrentLcid.os_filter_lcid = CultureInfo.CurrentUICulture.LCID.ToString();
             Console.WriteLine("Current lcid: {0}", cmdCurrentLcid.os_filter_lcid);
             cmdCurrentLcid.command = string.Format("cmd.exe /C dir > \"{0}\"", currentLcidFilename);
             cmdCurrentLcid.required_install = true;
@@ -64,7 +64,7 @@
             ConfigFile configFile = new ConfigFile();
             // current lcid setup configuration
             SetupConfiguration currentLcidConfiguration = new SetupConfiguration();
-            currentLcidConfiguration.lcid_filter = CultureInfo.CurrentCulture.LCID.ToString(); 
+            currentLcidConfiguration.lcid_filter = CultureInfo.CurrentUICulture.LCID.ToString(); 
             configFile.Children.Add(currentLcidConfiguration);
             string currentLcidFilename = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString());
             ComponentCmd cmdCurrentLcid = new ComponentCmd();
@@ -117,7 +117,7 @@
             string configFilename = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString() + ".xml");
             Console.WriteLine("Writing '{0}'", configFilename);
             configFile.SaveAs(configFilename);
-            Assert.AreEqual(CultureInfo.CurrentCulture.LCID, dotNetInstallerExeUtils.Run(configFilename));
+            Assert.AreEqual(CultureInfo.CurrentUICulture.LCID, dotNetInstallerExeUtils.Run(configFilename));
             File.Delete(configFilename);
         }
 
@@ -134,7 +134,7 @@
             string configFilename = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString() + ".xml");
             Console.WriteLine("Writing '{0}'", configFilename);
             configFile.SaveAs(configFilename);
-            Assert.AreEqual(CultureInfo.CurrentCulture.LCID, dotNetInstallerExeUtils.Run(configFilename));
+            Assert.AreEqual(CultureInfo.CurrentUICulture.LCID, dotNetInstallerExeUtils.Run(configFilename));
             File.Delete(configFilename);
         }
 

 

 

 

Coordinator
Jan 6, 2011 at 3:35 PM

You're absolutly right. The test tries to make sure that #LANGID is defined correctly. It's using a simpl trick of having a command componnt that exists with th #LANGID error code, and then checks that this is the error code returned. It should be the UI language ID, not the OS language ID (they are the same on my system, so I wrote a stupid test :)). Please commit this change, thx.