SNOW-893834 tests fix for Arrow #798
Conversation
Codecov Report
@@ Coverage Diff @@
## master #798 +/- ##
==========================================
+ Coverage 83.54% 84.01% +0.47%
==========================================
Files 89 89
Lines 9146 9146
Branches 835 835
==========================================
+ Hits 7641 7684 +43
+ Misses 1278 1241 -37
+ Partials 227 221 -6 see 3 files with indirect coverage changes 📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today! |
| @@ -997,6 +1003,8 @@ public void TestGetDataTypeName() | |||
| // Act | |||
| using (DbDataReader reader = (DbDataReader)cmd.ExecuteReader()) | |||
| { | |||
| ValidateResultFormat(reader); | |||
There was a problem hiding this comment.
this validation should be part of assert block
There was a problem hiding this comment.
I'm not sure because it is not something we want to test, it's only validation of proper arrange/setup. But if you think it'd be more clear, I'll do it.
| @@ -811,6 +815,8 @@ public void TestGetChar() | |||
| // Act | |||
| using (IDataReader reader = cmd.ExecuteReader()) | |||
| { | |||
| ValidateResultFormat(reader); | |||
There was a problem hiding this comment.
this validation should be part of assert block
There was a problem hiding this comment.
What do you think about Assert.IsTrue(reader.Read()) ?
I think it should be in Act not Assert block. But if so, should we leave Assert.IsTrue(...) or just call reader.Read() ?
There was a problem hiding this comment.
ValidateResultFormat is a wraper for:
Assert.AreEqual(_resultFormat, ((SnowflakeDbDataReader)reader).ResultFormat);
which is definitely an assert.
If you want you could split Assert.IsTrue(reader.Read()) into something like :
// act
var read = reader.Read();
// assert
Assert.IsTrue(read);
you can split that. However I would not do it, because for me "act" block should be "execute query on the server and get the result" and that's cmd.ExecuteReader() and checking the result is rather a part of assert for me.
| @@ -200,6 +200,8 @@ public void TestDateOutputFormat() | |||
| cmd.CommandText = $"select TO_DATE('2013-05-17')"; | |||
| IDataReader reader = cmd.ExecuteReader(); | |||
|
|
|||
| ValidateResultFormat(reader); | |||
|
|
|||
There was a problem hiding this comment.
you could even remove this empty line since it is one of assertions
|
|
||
| [ThreadStatic] | ||
| private static string t_directoryLogPath; | ||
| private string _directoryLogPath; |
There was a problem hiding this comment.
what was wrong with the previous form of thread static? It didn't work?
There was a problem hiding this comment.
There is no need to use static field so I removed it. Its because of: https://github.com/snowflakedb/snowflake-connector-net/actions/runs/6585609942/job/17892373465#step:8:4582
There was a problem hiding this comment.
This means that this test might be flaky. How your change is going to improve the situation? I think that the cause of the error is that we try to remove working directory after the test while logger has not flushed its logs to the file yet - or something like that. I don't see how your change will improve the situation.
There was a problem hiding this comment.
The process cannot access the file 'C:\Users\runneradmin\AppData\Local\Temp\easy_logging_logs_dkdsyynl.bgd\dotnet\snowflake_dotnet_k0201fjm.log' because it is being used by another process.
The problem might be connected with static field. Since there is no need to use static I changed it to private.
sfc-gh-dstempniak
force-pushed
the
SNOW-893834_2
branch
from
8037fa2
to
6260a69
Compare
Description
SNOW-893834 tests fix for Arrow
Checklist
dotnet test)