-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Fixes #2197 and #2195: Refresh admin page keeps it at same location; Add auto-scroll to show output in jobs tab. #2218
Conversation
@kevintab95 -- FYI, the Travis tests are failing! |
Current coverage is 48.01%@@ develop #2218 diff @@
==========================================
Files 185 185
Lines 15836 15857 +21
Methods 2718 2720 +2
Messages 0 0
Branches 2688 2692 +4
==========================================
Hits 7613 7613
- Misses 8223 8244 +21
Partials 0 0
|
The e2e test for the library suite passes on my local machine but fails the travis test. Can someone restart build? |
return window.location.hash; | ||
}, function(newHash) { | ||
switch (newHash) { | ||
case '#jobs': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we pull out these URLs as constants, and use them in the HTML code too (you'll need to change to using ng-href
)? This would help prevent people from changing the html and breaking the tab reloading.
@kevintab95 -- Thanks! I left some comments. |
@wxyxinyu I've made some changes like you suggested. Thanks! |
@@ -27,9 +27,32 @@ oppia.controller('Admin', ['$scope', '$http', function($scope, $http) { | |||
$scope.TAB_JOBS = 'TAB_JOBS'; | |||
$scope.TAB_CONFIG = 'TAB_CONFIG'; | |||
$scope.TAB_MISC = 'TAB_MISC'; | |||
$scope.jobsUrl = '#jobs'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The convention is to use all caps for constants, and you should do so here.
Just one small thing. This is great, it has been a bit annoying on the admin page for a while. Thanks for doing this! |
The e2e test for misc suite passes on my local machine but fails the Travis test, please restart the build. I wonder why this keeps happening? |
I think it's a spurious error. I restarted the test, hopefully it passes this time. The travis tests are sometimes a bit flaky haha. There are some issues tracking attempts to make tests less flaky e.g. #2042 |
Oh OK, thanks! and actually I noticed that it failed on my machine when i ran the test without the |
Yeah, sharding does lead to some flakyness, because the CPU load causes the e2e tests to run slower in some cases, and this causes some timeouts or clicks not being registered in the right place. But I generally think sharding is worth it, just because running without sharding takes 1h+ for all the e2e tests. The wiki page here https://github.com/oppia/oppia/wiki/Running-Tests#end-to-end-tests does describe some of the common problems and suggested solutions (especially the common e2e error messages that may indicate tests failed spuriously). |
Oh alright, cool! |
…location; Add auto-scroll to show output in jobs tab. (oppia#2218) * Fixes oppia#2197 and fixes oppia#2195 * fix e2e error (library) * Use URL as constants * Constants in all caps
Fixes #2197 and #2195: Refresh admin page keeps it at the same location; Auto-scroll for viewing output in jobs tab in admin panel.