Collection of work to improve efficiency of app #12081
Conversation
|
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
…derMan named migrateFolderDefinition improved loading from config to only save the extracted FolderDefintions if they were migrated started prepping for deeper cuts related to Folder::saveToSettings, plus overhaul of impl and uses of FolderMan::addFolderFromFolderWizardResult and addFolderFromWizard
|
Client crashed when squish tries to run client. Crash stracktrace and client logs are here: |
…ccessors, all members are now private, FolderMan has been removed as friend refactored addFolderInternal -> moved the small amount of actual folder adding code out and renamed it connectFolder. Also refactored unloadFolder -> moved some parts to removeFolderSync (formerly removeFolder which was easily confused with internal remove operations) and renamed it to disconnectFolder to match connectFolder activity. put an end to the extra careful handling of "unexpected" Vfs modes - basically now if the mode isn't among our legit set, it just assumes VfsMode = off added loads of todo's and other notes for next steps
…nto work/improveEfficiency
…nto work/improveEfficiency
ok param originally took a pointer to bool which makes no sense since it's a simple flag value, and most importantly not allowed to be nullptr. It should therefore be a ref.
Refactored the auto-loading of spaces/ocis folders on adding account. Moved the bulk of the activity to FolderMan where it belongs. saving is still wonky and done in the folder sometimes but now I should be able to finally finish this persistence task now that loading spaces/folders from account is in the FolderMan.
need to validate the auto-save on changing pause or vfs mode as I think we will still get too many saves when enabling/disabling vfs in the Folder but otherwise it's in good shape now last steps are to relocate the removeFromSettings operations from Folder to FolderMan
…n now there are a couple of wonky bits but they should be ironed out in future when we convert direct calls from Gui to FolderMan to signals requesting xyz change I have to do one more pass comparing functionality in AddFolderFromWizared to AddFolder, will tidy up and finally rename AddFolderFromWizard to something more sensible
we will first need adjust the .drone.star of all repos that use those secrets. We will track it in ticket owncloud/QA#883 |
|
aha! Excellent insight @saw-jan - that may make sense, since I did change how the folders are loaded from config but it should not be broken because of it, unless you are testing a really old config format or so. @ishabaral this may be important for you to know For example, migration/handling of legacy settings groups named Multifolders and FoldersWithPlaceholders is now gone, because this was already migrated in 5.0 and instances in those groups should no longer exist. If you can send me the config file this is breaking with I can have a look and try to see what is going on. I don't think I will be able to run with it, but sometimes just looking at it is enough. |
|
Tests generate these configs :
|
|
@ishabaral that is very helpful, thanks! so far, when I compare it to a real config created by the app using ocis, your test config is missing a lot of settings for the folders. I honestly have no idea if this is supposed to work without full settings. My gut tells me the complete set of settings should be present in any config file the app needs to use but let me look into it a bit. We have some local tests that use fake configs so once I find it, I'll compare to that to see what the "minimum" working config is. just for reference, here is the folder related content of my config that is syncing just one folder (created by the app, not generated) 0\default_sync_root=C:/Users/reeseThinkpad/ownCloud (3) |
|
I tried to use most of the possible configs like this. But still same issue. When using this config, I found out that this file |
|
I'm not sure what you mean - did adding the sync db entry to the config fix it? or do you mean that the app did not create the sync db entry in the config when you added the folder sync |
I mean that when starting the client this file: |
|
@ishabaral yes now I understand. I need to discuss this with my guys as the journal is created when the application adds folders from scratch (eg on connecting an account the first time or adding a folder sync by hand). the config is used to re-load folders which have been loaded from scratch in previous runs of the app, so it presumes the journals already exist. It assumes they live in the default location if the journal setting is empty, as it is for you, but it does not create the journal (since it should already exist). IMO it's not a correct test to use a config the way it's being used because it does not match real life purpose of the config file (which is to save data from previous runs). To fix this current issue, the app would have to handle not-real-life functionality just to support the tests which I don't think is the right way, but again I need to discuss this with the team. Primarily what I need to find out is if we offer some "feature" to users to set up accounts/syncs using a config file that is not generated by the app, or whether the app should try to "heal" or fix configurations that are somehow broken for whatever reason. I'll get back to you asap. Open question at the moment though: have you been using this type of config to create test scenarios for some time, right? Was that encouraged as a method of setting up the cases? |
I don't remember someone encouraging using the config file. But AFAIK, we used the config file to reduce the test run time by setting up the test accounts without having to go through the account setup dialog. We do have separate test scenarios for account setup dialog. Config file is used for tests that are not related to the account setup dialog. Let us know what will be the decision and then we can use the account setup flow for all test scenarios, and remove the config file usage |
|
@ishabaral or @saw-jan can you please provide a log file associated with this test failure? So far I don't see how this problem can arise so the log might help. |
|
@modSpike Here is the log file: |
|
Just a brief update after discussion with @erikjv: the current use of the "fake" config to bootstrap test cases is not ideal because it relies on behavior that is not officially supported for the user. In short the user needs to rely on the content of the config file which is generated by the app, and you should too, as "faking" it with a generated config can lead to missed bugs and false failures when we change code or features. Unfortunately it is not reasonable for us to intentionally support this corner use case for testing only, as it too often leads to confusing code, which we are trying to eliminate. So we'd like to open an issue to update the account bootstrapping on your side. We are happy to support you with this, and we already have some proposals to hopefully keep the setup simple while bringing the tests in line with "real world" use cases. The question is: should I open a general issue for this task or a dedicated QA issue? |
|
Thanks for the decision.
A dedicated QA issue would be great and then we can start workoing on that. |
|
ETA: I know you are ending your day over in Nepal - please look at this tomorrow! Regarding the log file results specifically which I'd still like to work out: It appears that the authentication with the credentials in the config fails with some ssl error? It looks like the test causes a dialog to pop asking for re-authentication. Then it can connect. Is this actually part of the test? When you re-authenticate, is it actually using the correct server which contains the folder you have specced in the config? Second, it appears that the test runs with local sync targets/folders that do not exist at all. It also appears that this is what causes the folder to not appear in the gui as from that point on, the folder loading fails so effectively, "no folder exists" to show there. Why this is, is still unclear to me as if I use a normal app generated config that points to a local folder that I have deleted, I still see the folder in the gui but it shows the local folder is missing and sync is blocked. Granted this is using windows/vfs so maybe there is a fundamental difference in how that is handled...still looking into it. Third, and very important: as I pointed out previously, the gui shows "Synchronizing 0 out of 0 Spaces" If this generated config "matches" the target server I would expect it to contain at least one folder, which would be reported as "syncronizing 0 out of 1 spaces" or something in that direction. the fact that the server is reporting 0 available spaces makes me think something is quite wrong. Will continue to look into possible fixes on my side but any additional info you can provide about these points would be really helpful. |
yeah, with pre-added config, we would see a cert accept dialog and have to re-authenticate when starting the client (as the config doesn't contain the cert and auth settings). The test would accept the cert and re-login again after starting the client and then perform the other checks.
Before starting the client, the test code will create the necessary sync paths. FYI, the test doesn't use the default sync and config paths, it uses the custom path in /tmp folder so that we don't mess with the existing personal sync folders. Line 1 in 157a2d7 |
Sure, we will give this a priority. Thank you |
…nto work/improveEfficiency
|
current state includes workarounds for the current qa configs in the FolderMan::migrateFolderDefinition those safety checks should be reevaluated and ideally removed once #12136 is resolved. |
Thanks a lot. we will let you know when the workaround can be removed. |
This branch will include some refactoring work to improve the efficiency of the app. First task is to reduce the number of calls to QSettings::sync(), and possibly refactor the update behavior of the FolderStatusModel