NH-4004 - Restrict tests running on SqlServer CE #621

hazzik · 2017-05-02T02:14:26Z

fredericDelaporte · 2017-05-02T08:33:21Z

src/NHibernate.Test/Criteria/CriteriaQueryTest.cs

@@ -112,6 +112,9 @@ public void ScrollCriteria()
 		[Test]
 		public void AllowToSetLimitOnSubqueries()
 		{
+			if (!TestDialect.SupportsNakedSubqueryInWhereClause)


I was not knowing about the TestDialects. I am not sure I like this pattern. They do not support inheritance. If someone want to test another version of a db than the ones we have put TestDialect for, he will encounter failures. While we already have the "informational metadata" in the dialects, which supports inheritance. Yes that is a kind of pollution since they are likely used only for tests. But for me they do not look as causing harm, while being a lot more versatile than the TestDialects.

Of course, when putting such properties in the Dialect class, we would have then to be a bit more explicit on their semantic, at least through xml comments. (By example: what would be the difference between SupportsNakedSubqueryInWhereClause and SupportsSubqueryInWhereClause (explained by the xml comment, so that is already fine currently); what are ComplexExpressionInGroupBy?)

fredericDelaporte · 2017-05-02T08:41:29Z

src/NHibernate.Test/Linq/QueryTimeoutTests.cs

+		protected override bool AppliesTo(Dialect.Dialect dialect)
+		{
+			//SqlServer CE does not support timeouts
+			return !(dialect is MsSqlCeDialect);


I prefer not introduce new "is that dialect" apply test, and rather add new "has such feature" test and property.

fredericDelaporte · 2017-05-02T08:48:44Z

src/NHibernate.Test/NHSpecificTest/NH1521/Fixture.cs

@@ -11,7 +11,8 @@ public class Fixture
 	{
 		private static void CheckDialect(Configuration configuration)
 		{
-			if (!configuration.Properties[Environment.Dialect].Contains("MsSql"))
+			var dialect = configuration.Properties[Environment.Dialect];
+			if (!dialect.Contains("MsSql") || dialect.Contains("MsSqlCe"))


That one is a "is this dialect" test too, but with how does work the test, it would be cumbersome to translate it to a "has that feature" test.

Now, I think it would be more straightforward to only test Dialect is MsSql2000Dialect, since all other SQL-Server dialects inherit it while MsSqlCe does not.

Dialect is not available here. This is a dialect specific test (I think it's MsSql2005/8Dialect), but unfortunately MsSqlCe dialects starts from the same string :)

Granted I have not checked it was inheriting from TestCase. But well, grabbing the dialect object looks simple: NHibernate.Dialect.Dialect.GetDialect(configuration.Properties).

fredericDelaporte · 2017-05-02T08:52:25Z

src/NHibernate.Test/NHSpecificTest/NH1635/Fixture.cs

@@ -7,6 +8,11 @@ namespace NHibernate.Test.NHSpecificTest.NH1635
 	[TestFixture]
 	public class Fixture : BugTestCase
 	{
+		protected override bool AppliesTo(Dialect.Dialect dialect)
+		{
+			return !(dialect is MsSqlCeDialect);


It should be TestDialect.SupportsSubqueryInSelectClause, shouldn't it? (The failing query does that and the error message is on the select keyword of the subquery.)

I probably did not add this property yet when I was making change to this file

fredericDelaporte · 2017-05-02T08:55:23Z

src/NHibernate.Test/NHSpecificTest/NH1925/Fixture.cs

+		protected override bool AppliesTo(Dialect.Dialect dialect)
+		{
+			// SqlServer CE does not support subquery in where clause
+			return !(dialect is MsSqlCeDialect);


It should be TestDialect.SupportsNakedSubqueryInWhereClause, shouldn't it? Or maybe we need to add a SupportsSubqueryInWhereClause here, but it looks to me the former is suitable.

fredericDelaporte · 2017-05-02T09:05:38Z

src/NHibernate.Test/ReadOnly/TextHolder.cs

@@ -10,7 +10,7 @@ public class TextHolder
 		/// </summary>
 		public static bool SupportedForDialect(Dialect.Dialect dialect)
 		{
-			return !(dialect is FirebirdDialect || dialect is Oracle8iDialect);
+			return !(dialect is FirebirdDialect || dialect is Oracle8iDialect || dialect is MsSqlCeDialect);


Here too, I would rather have a SupportsTextBlobType feature. This blacklisting may miss a bunch of supported db we are currently not testing. And when those text/ ntext types will start being dropped from newer version of SQL Server, the blacklisting will start to be cumbersome. (Granted, this looks like it may never happen, ntext and text are in the "will be removed in an undetermined future version" list since SQL 2005 till SQL 2016. Still waiting to see them in the "will be removed in next version list".)

By the way, character types looks like being another muddy place a bit alike dates types^^. (Even worse indeed.)

fredericDelaporte · 2017-05-02T09:36:54Z

I think it is a very welcome change overall.

fredericDelaporte · 2017-08-07T23:18:44Z

src/NHibernate.Test/Hql/Ast/BulkManipulation.cs

 using NHibernate.Hql.Ast.ANTLR;
-using NHibernate.Id;
-using NHibernate.Persister.Entity;
+using NHibernate.Linq;
 using NUnit.Framework;

 namespace NHibernate.Test.Hql.Ast
 {
 	[TestFixture]
 	public class BulkManipulation : BaseFixture


Heavily rewritten, as it was already done in #391: proper setup&teardown allowing to handle the "no temp table" case in which MsSqlCe falls.

fredericDelaporte · 2017-08-07T23:24:05Z

src/NHibernate/Dialect/MsSqlCeDialect.cs

@@ -339,6 +339,9 @@ public override long TimestampResolutionInTicks
 		/// </summary>
 		public override bool SupportsPoolingParameter => false;

+		/// <inheritdoc/>
+		public override bool SupportsScalarSubSelects => false;


Ms SQL CE supports only sub-queries defining a set, like with in where exists(sub-query), join (sub-query), from (sub-query), in (sub-query). Sub-queries to be handled as a scalar value (single column & single row) are not supported. So things like select (sub-query) as colName, where (sub-query) = someValue, order by (sub-query), ..., are not supported.

fredericDelaporte · 2017-08-07T23:27:23Z

src/NHibernate.Test/Hql/Ast/HqlFixture.cs

@@ -74,188 +74,95 @@ public void OrderByPropertiesImplicitlySpecifiedInTheSelect()
 			{
 				s.CreateQuery("select distinct z from Animal a join a.zoo as z order by z.name").List();
 			}
-		} 
+		}

 		[Test]
 		public void CaseClauseInSelect()


HqlFixture rewritten a bit like bul manipulation fixture, for the same reasons.

fredericDelaporte · 2017-08-07T23:28:04Z

src/NHibernate.Test/Hql/Ast/WithClauseFixture.cs

@@ -7,115 +6,88 @@ namespace NHibernate.Test.Hql.Ast
 	[TestFixture]
 	public class WithClauseFixture : BaseFixture


Rewritten a bit like bul manipulation fixture, for the same reasons.

fredericDelaporte · 2017-08-07T23:31:25Z

src/NHibernate.Test/Legacy/SQLLoaderTest.cs

@@ -333,8 +333,8 @@ public void EscapedODBC()
 		[Test]
 		public void DoubleAliasing()
 		{
-			if (Dialect is MySQLDialect) return;
-			if (Dialect is FirebirdDialect) return; // See comment below


Appears to work for them, probably since some time.

fredericDelaporte · 2017-08-07T23:39:24Z

src/NHibernate.Test/NHSpecificTest/NH1812/Fixture.cs

 using NUnit.Framework;

 namespace NHibernate.Test.NHSpecificTest.NH1812
 {
-	public class AstBugBase : BugTestCase
+	public class AstBug : BugTestCase


This test was originally having:

AstBugBase class without a Fixture attribute, not meant to be run (should have been further marked abstract for guaranteeing this).

AstBug, just deriving from AstBugBase with a Fixture attribute, meant to demonstrate the bug.

ItWorksWithClassicParser, deriving from AstBugBase with a Fixture attribute and alter the configuration for using the "classic parser". Meant to demonstrate this was a regression.

The later has been removed, rendering the inheritance scheme moot. But that was not cleaned. And NUnit changes caused the "base" class to be run too.

I have cleaned up that.

fredericDelaporte · 2017-08-08T08:24:38Z

src/NHibernate/Dialect/MsSqlCeDialect.cs


 			RegisterFunction("round", new StandardSQLFunction("round"));
+
+			RegisterFunction("bit_length", new SQLFunctionTemplate(NHibernateUtil.Int32, "datalength(?1) * 8"));
+			RegisterFunction("extract", new SQLFunctionTemplate(NHibernateUtil.Int32, "datepart(?1, ?3)"));


All these added functions were already tested (and previously failing) in HQLFunctions fixture.

fredericDelaporte · 2017-08-12T14:19:19Z

Since I have made notable additions on this PR, I would like to have your feedback @hazzik , rather than self approving then merging those additions.

And maybe doing a rebase before would be better.

hazzik · 2017-08-14T05:00:14Z

Can we have BulkManipulation refactoring as a separate request? It's not clear what's going on there.

fredericDelaporte · 2017-08-14T08:26:48Z

I will not be clearer with a separate request. It will just cause more overhead to tackle those pile of badly written tests. First it will force to wholly disable them for SqlServer Ce if we are aiming at eliminating tests failures with this current PR. Then it will cause re-doing the work in a separate request for allowing testing DML operations for SqlServer Ce too.

hazzik · 2017-08-14T08:28:13Z

So, refactoring PR first. Then this PR. I think this way it would work.

fredericDelaporte · 2017-08-14T08:34:25Z

Sorry, I do not see enough added value in proceeding that way. All NHibernate subjects here takes way to much time to tackle due in part to what I see as overdue nitpicking. No wonder NHibernate project is unable to foster collaboration and is so much lacking active contributors.

hazzik · 2017-08-14T08:38:14Z

https://en.wikipedia.org/wiki/Correlation_does_not_imply_causation

hazzik · 2017-08-14T08:46:31Z

nitpicking is because the subjects are complex lacking active contributors because the herd has moved to EF and because the subjects are complex. PS: I don't agree that these tests are badly written, They are ported from Java and Java did not have a concept of try-with-resource (aka using in C#) prior 1.8. So it is written this way. Refactoring and changing the functionality in the same commit is a bad habit as it can cause errors.

fredericDelaporte · 2017-08-14T11:57:47Z

Everything can cause errors, we are just humans. But that is not a trouble, errors are recoverable.

For me there is a trouble with the pace at which we can contribute to NHibernate. I find it sluggish. Error rate is maybe very low, but at an excessive contributing time cost in my opinion. And as showcased by the VS2017 csproj format migration, there are still errors. But well, that can be fixed (and already is for this example).

At some point trying hard to have a very low error rate can end up costing way more time than letting more errors happen then fixing them.

About the refactoring itself in a separated PR, I would have agreed if it was about a feature. But here this is just fixing tests, and it has its own commit. A PR for refactoring one test fixture as a prerequisite to actually fixing a trouble, instead of doing it all in one PR, is overkill in my opinion.

The change is not just about try catch/using, these were just a "bonus". With the way you seems to consider such changes, I should not have done them.
The main point is the isolation of data setup and tear-down in their NUnit ad-hoc methods. Instead of letting them embedded in each test, causing tear-down to be skipped in case of test failure. And by the way wholly consolidating those setup and tear-down logic in one single place, the previous TestData class logic covering only part of it. Without that the special tear-down logic required for Sql Server Ce would have required to be duplicated in many tests.
Is JUnit without counterparts for these methods too?

Another trouble with those tests, which I have not tackled, is their tendency to test many different things in each test, causing them to get some "if supported then additionally test that" logic, which looks bad. Better have many more tests with some of them fully ignored because fully unsupported, rather than having fewer and almost all successful (rather than ignored) tests, but having ignored some parts of them without emitting much clues about that from the test framework perspective.
That is why I wrote those tests are badly written, and it does not look to me those points are due to Java constraints.

hazzik · 2017-08-14T12:17:52Z

and it has its own commit.

I don't see this commit. I would be OK with the separate commit for the refactoring and a separate commit for disabling irrelevant tests.

But here this is just fixing tests

Tests are crucial to software, they are not "just tests". I've seen many times how such "refactorings" made test incorrect and because these errors were accompanied by feature changes they went unnoticed for a long time.

What I'm trying to say: bigger commits require longer time to review. Changes which Github decides to collapse because they are too big to be displayed require the infinite amount of time.

Didn't comment anything else because it's almost bed time and I don't want to spend the night arguing.

hazzik · 2017-08-14T12:41:45Z

I checked some of the changes of BulkManipulation and I completely disagree with them. A couple of tests have completely changed semantic now. Also, for some of the tests, the test routine has moved to the setup, which made these tests green but pointless. From the TC log, I can see that almost only "negative" tests (the tests checking that incorrect usage will throw) are run for Sql CE.

Now I think that this is our best option:

to wholly disable them for SqlServer Ce

fredericDelaporte · 2017-08-14T19:15:57Z

A couple of tests have completely changed semantic now. Also, for some of the tests, the test routine has moved to the setup, which made these tests green but pointless.

These are serious statements. I do not believe to have made such mistakes. Please back such statements with pointers to at least some sample of these tests you consider affected by such mistakes.

Many tests are disabled for sql server ce, specifically all the ones relying on multiple tables operation (join mapping). But other more usual cases (class mapped to a single table) do work and are tested.

Anyway, if you rather "fix" that by disabling dml tests wholly for sql server ce, drop my commit and go ahead.

hazzik · 2017-08-14T22:21:18Z

InsertWithGeneratedId, InsertWithGeneratedVersionAndId, InsertWithGeneratedTimestampVersion and probably some other

fredericDelaporte · 2017-08-15T07:47:54Z

Ok, so that is the filter for avoiding the old Firebird "instable cursor" bug that you consider as a semantic change I guess. Not untrue, but the fact that the query was filtered or not was not IMO the subject of the test, and was an external issue, not a one NHibernate could fix. Anyway this Firebird workaround could be no more needed with the upgrade to 3.0 done some PR ago, so it may be rewritten without it. This workaround was coming from the changes already done in strongly typed DML PR (as of most of the other changes in this file).

Anyway, enough with that subject, I am probably going to drop that commit. (So force update will be required, so will rebase by the way, but due to having limited connectivity currently I am a bit stuck with VS 2017.2 for now and will have to wait.)
I have checked gain which tests remain for SQL Server Ce, and there is at least one for each operation (insert/update/delete) actually performing the operation, not simply testing an error case. So just disabling them for SQL Server CE is a bit unfortunate, unless we already consider sun-setting its support (as I was suggested some time ago).
(Granted, this is a bit obfuscated by the fact that most entities in those tests are not simple ones, so few tests actually check the basic and probably most usual use cases.)

fredericDelaporte

Hql/Ast/HqlFixture and Hql/Ast/WithClauseFixture were re-written similarly to BulkManipulation. These classes are less big so it was not as big a change than for BulkManipulation. Do you want them to be reverted too?

fredericDelaporte · 2017-08-18T06:50:52Z

src/NHibernate.Test/Hql/Ast/HqlFixture.cs

+						.CreateCriteria<Animal>()
+						.SetProjection(Projections.RowCount())
+						.Add(Restrictions.Gt("bodyWeight", 5.5f))
+						.Add(Restrictions.Lt("bodyWeight", 6f))


Additional restriction added because more data initialization is now consolidated in the setup, and this condition allows to filter it out.

fredericDelaporte · 2017-08-19T21:16:02Z

Do you want them to be reverted too?

Now done anyway.

hazzik · 2017-08-19T21:17:41Z

Can we focus on AsyncGenerator and Linq DML instead?

…

On Sun, 20 Aug 2017 at 9:16 AM, Frédéric Delaporte ***@***.***> wrote: Do you want them to be reverted too? Now done anyway. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#621 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAI1l7NxGowKSH-VXwk9wjM2CV0FErv2ks5sZ1CSgaJpZM4NNowo> .

fredericDelaporte · 2017-08-19T21:22:53Z

No feed back on Linq DML, nothing more te be done on my side without any responses from others.
Asyncgenerator: well that is precisely for this one that I am doing all those tests cleanup: that should remove some of those async failing tests due to their sync counterpart failing too.

…mporary tables.

hazzik · 2017-08-20T02:44:18Z

Thanks. I've recombined the commits into 4.

hazzik

LGTM.

hazzik force-pushed the NH-4004 branch 3 times, most recently from 55cb6f1 to b4dd864 Compare May 2, 2017 05:37

hazzik requested a review from fredericDelaporte May 2, 2017 08:51

fredericDelaporte reviewed May 2, 2017

View reviewed changes

hazzik added the needs work label May 2, 2017

hazzik modified the milestone: 5.0 Aug 3, 2017

hazzik added the t: Improvement label Aug 3, 2017

fredericDelaporte reviewed Aug 7, 2017

View reviewed changes

fredericDelaporte previously approved these changes Aug 8, 2017

View reviewed changes

fredericDelaporte removed the needs work label Aug 8, 2017

fredericDelaporte reviewed Aug 8, 2017

View reviewed changes

fredericDelaporte dismissed their stale review via 160c16e August 18, 2017 07:54

fredericDelaporte force-pushed the NH-4004 branch from ee87fe5 to 160c16e Compare August 18, 2017 07:54

fredericDelaporte reviewed Aug 18, 2017

View reviewed changes

fredericDelaporte force-pushed the NH-4004 branch from 160c16e to f45b7a9 Compare August 19, 2017 21:15

fredericDelaporte added 4 commits August 20, 2017 14:41

NH-4004 - Restrict tests running on SqlServer CE

938cbc6

NH-4004 - Disabling Bulk manipulations for dialects not supporting te…

fb079f2

…mporary tables.

NH-4070 - Add missing function overrides to MsSqlCe

bb0249b

NH-4004 - Removing support for failed tests with MsSqlCe

c3526ee

hazzik force-pushed the NH-4004 branch from f45b7a9 to c3526ee Compare August 20, 2017 02:42

hazzik commented Aug 20, 2017

View reviewed changes

fredericDelaporte approved these changes Aug 20, 2017

View reviewed changes

fredericDelaporte merged commit 675ec72 into nhibernate:master Aug 20, 2017

hazzik deleted the NH-4004 branch August 25, 2017 01:50

hazzik added the r: Fixed label Aug 30, 2017

		@@ -7,115 +6,88 @@ namespace NHibernate.Test.Hql.Ast
		[TestFixture]
		public class WithClauseFixture : BaseFixture

NH-4004 - Restrict tests running on SqlServer CE #621

NH-4004 - Restrict tests running on SqlServer CE #621

Uh oh!

Conversation

hazzik commented May 2, 2017 • edited by fredericDelaporte Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte May 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte commented May 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Aug 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte commented Aug 12, 2017

Uh oh!

hazzik commented Aug 14, 2017

Uh oh!

fredericDelaporte commented Aug 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hazzik commented Aug 14, 2017

Uh oh!

fredericDelaporte commented Aug 14, 2017

Uh oh!

hazzik commented Aug 14, 2017 via email

Uh oh!

hazzik commented Aug 14, 2017 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fredericDelaporte commented Aug 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hazzik commented Aug 14, 2017

Uh oh!

hazzik commented Aug 14, 2017

Uh oh!

fredericDelaporte commented Aug 14, 2017

Uh oh!

hazzik commented Aug 14, 2017

Uh oh!

fredericDelaporte commented Aug 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fredericDelaporte left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hazzik commented May 2, 2017 •

edited by fredericDelaporte

Loading

fredericDelaporte May 2, 2017 •

edited

Loading

fredericDelaporte Aug 8, 2017 •

edited

Loading

fredericDelaporte commented Aug 14, 2017 •

edited

Loading

hazzik commented Aug 14, 2017 via email •

edited

Loading

fredericDelaporte commented Aug 14, 2017 •

edited

Loading

fredericDelaporte commented Aug 15, 2017 •

edited

Loading