Fix functional tests #3501
Fix functional tests #3501
Conversation
joelverhagen
requested review from
shishirx34,
chenriksson,
cristinamanum,
skofman1 and
scottbommarito
| private static List<string> _trustedHttpsCertificates; | ||
|
|
||
| /// <summary> | ||
| /// Option to enable or disable functional tests from the current run. | ||
| /// </summary> | ||
| public static string RunFunctionalTests | ||
| public static bool RunFunctionalTests |
There was a problem hiding this comment.
what is the use case for this setting?
There was a problem hiding this comment.
The gallery test fixture requires that this environment variable is set to true to run:
There was a problem hiding this comment.
But why will we ever want to turn it off?
There was a problem hiding this comment.
This setting is not necessary. Removed.
| } | ||
| } | ||
| bool runFunctionalTests; | ||
| if (!bool.TryParse(Environment.GetEnvironmentVariable("RunFunctionalTests"), out runFunctionalTests)) |
There was a problem hiding this comment.
We should be able to distinguish between the cases:
- Config is empty (null or empty string)
- TryParse fails because the value is not "true"/"false"
Also, consider if you want to lowercase the value before parsing. If I remember correctly, "True" might fail to be parsed
There was a problem hiding this comment.
We should be able to distinguish between the cases:
Why?
Also, consider if you want to lowercase the value before parsing. If I remember correctly, "True" might fail to be parsed
Nope, bool.TryParse accepts different casings and whitespace prefix/suffix.
https://dotnetfiddle.net/DijWeC
There was a problem hiding this comment.
- if the config is empty, the user didn't add a value to the setting, and we should fallback to default behavior (what ever we decide it is).
If the user wrote "blablabla" in the setting, we need to fail on parsing, and show that we failed on parsing, instead of swallowing the error.
There was a problem hiding this comment.
I think that depends on the case. But I'm pretty sure we can just remove this setting completely.
There was a problem hiding this comment.
By the way the previous implementation only reacted to "True" case-insensitive. It fell back to false for "blablablah".
There was a problem hiding this comment.
This setting is not necessary. Removed.
| using System.Collections.Generic; | ||
| using Microsoft.VisualStudio.TestTools.WebTesting; | ||
| using NuGetGallery.FunctionalTests.Helpers; | ||
|
|
||
| namespace NuGetGallery.FunctionalTests.WebUITests.ReadOnlyMode | ||
| { | ||
| /// <summary> | ||
| /// Go Account management activities like "Unsunscribe" notications and "Reset" Api key in read-only mode and check for error. | ||
| /// Go Account management activities like "Unsubscribe" notications and "Reset" Api key in read-only mode and check for error. |
UploadPackageFromUIwas improperly asserting 503 against a non-read-only server.ReadOnlyflag. This is the wrong way to skip tests.Addresses https://github.com/NuGet/Engineering/issues/188.
Found https://github.com/NuGet/Engineering/issues/200 in the process. Will address separately.