search

TEAMMATES / teammates

This is the project website for the TEAMMATES feedback management tool for education
https://teammatesv4.appspot.com/
GNU General Public License v2.0
1.67k stars 3.3k forks source link

Remove MashupPageUiTest #7926

Closed damithc closed 6 years ago

damithc commented 7 years ago

The mashup page is badly broken and the test is not used for its intended purpose anyway. Lets get rid of it.

damithc commented 7 years ago

If somebody can report here exactly what needs to be deleted, we can downgrade this to a FirstTimer issue.

amarlearning commented 7 years ago

I'm not 100% sure, but I think removing

will remove the MashupPageUiTest.

AlexM9200 commented 7 years ago

Can I try this?

LiHaoTan commented 7 years ago

@AlexM9200 sure you can but there are still a few more things to remove/modify other than what @amarlearning mentioned. If you can figure them out go ahead otherwise maybe you should do a d.FirstTimers first instead.

AlexM9200 commented 7 years ago

OK thanks i'll have a look. If I don't make a pull request within a few hours someone else can have it.

wkurniawan07 commented 7 years ago

I can tell which files need to be removed, but before that, let's get some things straight first:

The mashup page is badly broken

I just checked, it is indeed broken but has a simple two-liner fix.

The test is not used for its intended purpose anyway

I never asked this before: what is the intended purpose to begin with?

damithc commented 7 years ago

If it is to manually eyeball the page (as suggested by the introduction), then this is not a "test" per se, but more like some kind of script to be run from time to time. It may be worth to be moved to scripts instead.

Yes, it is for manual eyeballing of the entire UI in one page. Previously, it used to be the last test to run and leaves the page loaded at the end of the test suite for me to inspect before I set the new version live. That no longer happens and I don't think I've inspected this page since many moons. :-p

If the maintenance cost is worth the effort, I don't mind keeping it. e.g. everytime we add a new page, we should add it to the mashup as well.

wkurniawan07 commented 7 years ago

Previously, it used to be the last test to run and leaves the page loaded at the end of the test suite for me to inspect before I set the new version live. That no longer happens

This is because the said test is excluded from CI tests or staging tests (before it was removed). Possible solutions here:

If the maintenance cost is worth the effort, I don't mind keeping it. e.g. everytime we add a new page, we should add it to the mashup as well.

I will defer this decision to the new team, but judging from the number of new pages added in recent 2-3 years (less than 5?), it should not be a big issue.

I will, however, reiterate my stand that at its current state, this cannot be called a "test".

whipermr5 commented 7 years ago

The mashup page is badly broken

How is the page supposed to look like? I've only ever known it in its current state. I'm guessing all the elements should be styled exactly as they appear in the actual pages?

wkurniawan07 commented 7 years ago

@whipermr5 change the following two lines:

https://github.com/TEAMMATES/teammates/blob/3d248661dd3daa4d4fb1843f795b43bc708efed5/src/main/webapp/test/mashup.jsp#L18-L19

to:

-     <link type="text/css" href="<%= FrontEndLibrary.BOOTSTRAP_CSS %>">
-     <link type="text/css" href="<%= FrontEndLibrary.BOOTSTRAP_THEME_CSS %>">
+     <link type="text/css" href="<%= FrontEndLibrary.BOOTSTRAP_CSS %>" rel="stylesheet">
+     <link type="text/css" href="<%= FrontEndLibrary.BOOTSTRAP_THEME_CSS %>" rel="stylesheet">

That concludes what I said as "simple two-liner fix".

damithc commented 7 years ago

How is the page supposed to look like? I've only ever known it in its current state. I'm guessing all the elements should be styled exactly as they appear in the actual pages?

Yes.

orinn248 commented 7 years ago

Hey, has a decision been made on the appropriate approach or is this up to whoever submits the changes?

whipermr5 commented 7 years ago

Let's fix it and move it into client scripts.

LiHaoTan commented 7 years ago

I'm just wondering if it has much use though? Sounds like it's more like a maintenance burden with little benefits.

sukanta-27 commented 7 years ago

@damithc @wkurniawan07 @whipermr5 @LiHaoTan Has any decision been made regarding this issue?

wkurniawan07 commented 7 years ago

I'm just wondering if it has much use though

@LiHaoTan you can say the same for more than half of the current client scripts 😛

damithc commented 6 years ago

Putting on hold for now