Skip to content

Remove MusicStore (E2E tests have been disabled since Mar 2019) #26940

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
pranavkm opened this issue Oct 15, 2020 · 6 comments · Fixed by #27853
Closed

Remove MusicStore (E2E tests have been disabled since Mar 2019) #26940

pranavkm opened this issue Oct 15, 2020 · 6 comments · Fixed by #27853
Assignees
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates Done This issue has been fixed task
Milestone

Comments

@pranavkm
Copy link
Contributor

https://github.com/dotnet/aspnetcore/blob/master/src/MusicStore/test/MusicStore.E2ETests/MusicStore.E2ETests.csproj#L10.

@pranavkm pranavkm added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Oct 15, 2020
@mkArtakMSFT
Copy link
Contributor

Given that these tests have not been running for this long, they seem to add no value. We already have alternative mechanisms for testing.
@BrennanConroy, @Tratcher, @davidfowl let us know if you have a good argument against this.

@mkArtakMSFT mkArtakMSFT added this to the Backlog milestone Oct 16, 2020
@ghost
Copy link

ghost commented Oct 16, 2020

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@mkArtakMSFT mkArtakMSFT changed the title MusicStore E2E tests have been disabled since Mar 2019 Remove MusicStore (E2E tests have been disabled since Mar 2019) Oct 16, 2020
@Tratcher
Copy link
Member

We already have alternative mechanisms for testing.

What else covers this level of integration testing?

@pranavkm
Copy link
Contributor Author

CTI tests have been used to capture some of the E2E scenarios we previously had with MusicStore. Are there scenarios these tests cover that aren't captured by CTI (I know for example targeting specific operating systems such as Windows Server Nano is one of them)? If they are reasonable we should consider adding CTI coverage for it.

@mkArtakMSFT mkArtakMSFT modified the milestones: Backlog, 6.0-preview1 Nov 12, 2020
@Tratcher
Copy link
Member

FYI there's an outstanding ask to remove the OAuth credentials from the app because they're trigger automated alerts.

.AddFacebook(options =>
{
options.AppId = "550624398330273";
options.AppSecret = "10e56a291d6b618da61b1e0dae3a8954";
})
.AddGoogle(options =>
{
options.ClientId = "995291875932-0rt7417v5baevqrno24kv332b7d6d30a.apps.googleusercontent.com";
options.ClientSecret = "J_AT57H5KH_ItmMdu0r6PfXm";
})
.AddTwitter(options =>
{
options.ConsumerKey = "lDSPIu480ocnXYZ9DumGCDw37";
options.ConsumerSecret = "fpo0oWRNc3vsZKlZSq1PyOSoeXlJd7NnG4Rfc94xbFXsdcc3nH";
})
// The MicrosoftAccount service has restrictions that prevent the use of
// http://localhost:5001/ for test applications.
// As such, here is how to change this sample to uses http://ktesting.com:5001/ instead.
// From an admin command console first enter:
// notepad C:\Windows\System32\drivers\etc\hosts
// and add this to the file, save, and exit (and reboot?):
// 127.0.0.1 ktesting.com
// Then you can choose to run the app as admin (see below) or add the following ACL as admin:
// netsh http add urlacl url=http://ktesting:5001/ user=[domain\user]
// The sample app can then be run via:
// dnx . web
.AddMicrosoftAccount(options =>
{
// MicrosoftAccount requires project changes
options.ClientId = "000000004012C08A";
options.ClientSecret = "GaMQ2hCnqAC6EcDLnXsAeBVIJOLmeutL";
});

We should just remove the whole app at this point.

@blowdart
Copy link
Contributor

Agree with Chris, it's simpler from a compliance perspective to kill it, making it compliant would involve moving creds to an approved secret mechanism, and then putting the creds for the secret mechanism in a deployment pipeline which you'd need to keep up to date.

@mkArtakMSFT / @pranavkm if this could be killed as a matter of urgency it'd make life easier for the MSRC threat hunting folks to concentrate on real issue.

@mkArtakMSFT mkArtakMSFT linked a pull request Nov 15, 2020 that will close this issue
@ghost ghost added Done This issue has been fixed and removed Working labels Nov 16, 2020
@mkArtakMSFT mkArtakMSFT self-assigned this Nov 16, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates Done This issue has been fixed task
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants